Fix `Socket.ConnectAsync(SocketAsyncEventArgs)` behavior. (#15947)

We need to clearly distinguish between synchronous and asynchronous completions
and avoid any ambiguities and race conditions when it comes to error handling.

On synchronous completion, we must not invoke the async completion delegate.

In `System.Net.Sockets/Socket.cs`, the internal `BeginSConnect()` and `BeginMConnect()`
now return a `bool` indicating whether or not an async operation is pending.

If any error happens prior to starting the async operation, then we shall call
`SocketAsyncResult.Complete(..., true)` and return `false`.  And since this is
a synchronous completion, it's `AsyncCallback` will not be invoked.

In `referencesource/System/net/System/Net/Sockets/_MultipleConnectAsync.cs`,
I compared the entire file against the CoreFX version and then copied the
`AttemptConnection()` method from their implementation to adapt their behavior.

The difference is that on synchronous completion, the `InternalConnectCallback(null, args)`
should be invoked.

3 files changed, 65 insertions, 18 deletions

diff --git a/mcs/class/System/System.Net.Sockets/Socket.cs b/mcs/class/System/System.Net.Sockets/Socket.cs
index 39b001337b0..00fb6303434 100644
--- a/mcs/class/System/System.Net.Sockets/Socket.cs
+++ b/mcs/class/System/System.Net.Sockets/Socket.cs
@@ -912,20 +912,61 @@ namespace System.Net.Sockets
 			try {
 				IPAddress [] addresses;
 				SocketAsyncResult ares;
+				bool pending;
+
+				/*
+				 * Both BeginSConnect() and BeginMConnect() now return a `bool` indicating whether or
+				 * not an async operation is pending.
+				 */
 
 				if (!GetCheckedIPs (e, out addresses)) {
 					//NOTE: DualMode may cause Socket's RemoteEndpoint to differ in AddressFamily from the
 					// SocketAsyncEventArgs, but the SocketAsyncEventArgs itself is not changed
-					ares = (SocketAsyncResult) BeginConnect (e.RemoteEndPoint, ConnectAsyncCallback, e);
+
+					ares = new SocketAsyncResult (this, ConnectAsyncCallback, e, SocketOperation.Connect) {
+						EndPoint = e.RemoteEndPoint
+					};
+
+					pending = BeginSConnect (ares);
 				} else {
 					DnsEndPoint dep = (DnsEndPoint)e.RemoteEndPoint;
-					ares = (SocketAsyncResult) BeginConnect (addresses, dep.Port, ConnectAsyncCallback, e);
+
+					if (addresses == null)
+						throw new ArgumentNullException ("addresses");
+					if (addresses.Length == 0)
+						throw new ArgumentException ("Empty addresses list");
+					if (this.AddressFamily != AddressFamily.InterNetwork && this.AddressFamily != AddressFamily.InterNetworkV6)
+						throw new NotSupportedException ("This method is only valid for addresses in the InterNetwork or InterNetworkV6 families");
+					if (dep.Port <= 0 || dep.Port > 65535)
+						throw new ArgumentOutOfRangeException ("port", "Must be > 0 and < 65536");
+
+					ares = new SocketAsyncResult (this, ConnectAsyncCallback, e, SocketOperation.Connect) {
+						Addresses = addresses,
+						Port = dep.Port,
+					};
+
+					is_connected = false;
+
+					pending = BeginMConnect (ares);
 				}
 
-				if (ares.IsCompleted && ares.CompletedSynchronously) {
-					ares.CheckIfThrowDelayedException ();
-					return false;
+				if (!pending) {
+					/*
+					 * On synchronous completion, the async callback will not be invoked.
+					 *
+					 * We need to call `EndConnect ()` here to close the socket and make sure
+					 * that any pending exceptions are properly propagated.
+					 *
+					 * Note that we're not calling `e.Complete ()` (or resetting `e.in_progress`) here.
+					 */
+					e.current_socket.EndConnect (ares);
 				}
+
+				return pending;
+			} catch (SocketException exc) {
+				e.SocketError = exc.SocketErrorCode;
+				e.socket_async_result.Complete (exc, true);
+				return false;
 			} catch (Exception exc) {
 				e.socket_async_result.Complete (exc, true);
 				return false;
@@ -1044,7 +1085,7 @@ namespace System.Net.Sockets
 			return sockares;
 		}
 
-		static void BeginMConnect (SocketAsyncResult sockares)
+		static bool BeginMConnect (SocketAsyncResult sockares)
 		{
 			Exception exc = null;
 
@@ -1053,17 +1094,18 @@ namespace System.Net.Sockets
 					sockares.CurrentAddress++;
 					sockares.EndPoint = new IPEndPoint (sockares.Addresses [i], sockares.Port);
 
-					BeginSConnect (sockares);
-					return;
+					return BeginSConnect (sockares);
 				} catch (Exception e) {
 					exc = e;
 				}
 			}
 
+			sockares.Complete (exc, true);
+			return false;
 			throw exc;
 		}
 
-		static void BeginSConnect (SocketAsyncResult sockares)
+		static bool BeginSConnect (SocketAsyncResult sockares)
 		{
 			EndPoint remoteEP = sockares.EndPoint;
 			// Bug #75154: Connect() should not succeed for .Any addresses.
@@ -1071,14 +1113,15 @@ namespace System.Net.Sockets
 				IPEndPoint ep = (IPEndPoint) remoteEP;
 				if (ep.Address.Equals (IPAddress.Any) || ep.Address.Equals (IPAddress.IPv6Any)) {
 					sockares.Complete (new SocketException ((int) SocketError.AddressNotAvailable), true);
-					return;
+					return false;
 				}
 
 				sockares.EndPoint = remoteEP = sockares.socket.RemapIPEndPoint (ep);
 			}
 
 			if (!sockares.socket.CanTryAddressFamily(sockares.EndPoint.AddressFamily)) {
-				throw new ArgumentException(SR.net_invalidAddressList);
+				sockares.Complete (new ArgumentException(SR.net_invalidAddressList), true);
+				return false;
 			}
 
 			int error = 0;
@@ -1090,8 +1133,10 @@ namespace System.Net.Sockets
 				sockares.socket.connect_in_progress = false;
 				sockares.socket.m_Handle.Dispose ();
 				sockares.socket.m_Handle = new SafeSocketHandle (sockares.socket.Socket_internal (sockares.socket.addressFamily, sockares.socket.socketType, sockares.socket.protocolType, out error), true);
-				if (error != 0)
-					throw new SocketException (error);
+				if (error != 0) {
+					sockares.Complete (new SocketException (error), true);
+					return false;
+				}
 			}
 
 			bool blk = sockares.socket.is_blocking;
@@ -1106,7 +1151,7 @@ namespace System.Net.Sockets
 				sockares.socket.is_connected = true;
 				sockares.socket.is_bound = true;
 				sockares.Complete (true);
-				return;
+				return false;
 			}
 
 			if (error != (int) SocketError.InProgress && error != (int) SocketError.WouldBlock) {
@@ -1114,7 +1159,7 @@ namespace System.Net.Sockets
 				sockares.socket.is_connected = false;
 				sockares.socket.is_bound = false;
 				sockares.Complete (new SocketException (error), true);
-				return;
+				return false;
 			}
 
 			// continue asynch
@@ -1123,6 +1168,7 @@ namespace System.Net.Sockets
 			sockares.socket.connect_in_progress = true;
 
 			IOSelector.Add (sockares.Handle, new IOSelectorJob (IOOperation.Write, BeginConnectCallback, sockares));
+			return true;
 		}
 
 		static IOAsyncCallback BeginConnectCallback = new IOAsyncCallback (ares => {
diff --git a/mcs/class/System/System.Net.Sockets/SocketAsyncResult.cs b/mcs/class/System/System.Net.Sockets/SocketAsyncResult.cs
index 3f5faef9bd6..4004e475854 100644
--- a/mcs/class/System/System.Net.Sockets/SocketAsyncResult.cs
+++ b/mcs/class/System/System.Net.Sockets/SocketAsyncResult.cs
@@ -153,7 +153,7 @@ namespace System.Net.Sockets
 			Socket completedSocket = socket;
 			SocketOperation completedOperation = operation;
 
-			if (this.AsyncCallback != null) {
+			if (!CompletedSynchronously && AsyncCallback != null) {
 				ThreadPool.UnsafeQueueUserWorkItem(state => ((SocketAsyncResult)state).AsyncCallback((SocketAsyncResult)state), this);
 			}
 
diff --git a/mcs/class/System/Test/System.Net.Sockets/SocketTest.cs b/mcs/class/System/Test/System.Net.Sockets/SocketTest.cs
index 3dc69374874..32adec28b20 100755
--- a/mcs/class/System/Test/System.Net.Sockets/SocketTest.cs
+++ b/mcs/class/System/Test/System.Net.Sockets/SocketTest.cs
@@ -4707,9 +4707,10 @@ namespace MonoTests.System.Net.Sockets
 			socketArgs.RemoteEndPoint = endPoint;
 			socketArgs.Completed += (sender, e) => mre.Set ();
 
-			socket.ConnectAsync (socketArgs);
+			if (socket.ConnectAsync (socketArgs))
+				Assert.IsTrue (mre.WaitOne (1000), "ConnectedAsync timeout");
 
-			Assert.IsTrue (mre.WaitOne (1000), "ConnectedAsync timeout");
+			Assert.AreNotEqual (SocketError.Success, socketArgs.SocketError);
 		}
 
 		[Test] // Covers https://bugzilla.xamarin.com/show_bug.cgi?id=52549