Fix `Socket.ConnectAsync(SocketAsyncEventArgs)` behavior. (#15948)mono-6.0.0.319

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.

Backport of #15947.

4 files changed, 93 insertions, 26 deletions

diff --git a/mcs/class/System/System.Net.Sockets/Socket.cs b/mcs/class/System/System.Net.Sockets/Socket.cs
index a26cef315e9..ab15f1ed12a 100644
--- a/mcs/class/System/System.Net.Sockets/Socket.cs
+++ b/mcs/class/System/System.Net.Sockets/Socket.cs
@@ -911,20 +911,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;
@@ -1018,7 +1059,7 @@ namespace System.Net.Sockets
 			return sockares;
 		}
 
-		static void BeginMConnect (SocketAsyncResult sockares)
+		static bool BeginMConnect (SocketAsyncResult sockares)
 		{
 			Exception exc = null;
 
@@ -1027,17 +1068,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.
@@ -1045,14 +1087,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;
@@ -1064,8 +1107,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;
@@ -1080,7 +1125,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) {
@@ -1088,7 +1133,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
@@ -1097,6 +1142,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 14976edb39c..4d55ce80fb7 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
diff --git a/mcs/class/referencesource/System/net/System/Net/Sockets/_MultipleConnectAsync.cs b/mcs/class/referencesource/System/net/System/Net/Sockets/_MultipleConnectAsync.cs
index b8a4275b4e2..3d623fac590 100644
--- a/mcs/class/referencesource/System/net/System/Net/Sockets/_MultipleConnectAsync.cs
+++ b/mcs/class/referencesource/System/net/System/Net/Sockets/_MultipleConnectAsync.cs
@@ -219,28 +219,48 @@ namespace System.Net.Sockets
         {
             try
             {
-                Socket attemptSocket = null;
+                Socket attemptSocket;
                 IPAddress attemptAddress = GetNextAddress(out attemptSocket);
 
                 if (attemptAddress == null)
                 {
-                    return new SocketException(SocketError.NoData);
+                    return new SocketException((int)SocketError.NoData);
                 }
 
-                GlobalLog.Assert(attemptSocket != null, "MultipleConnectAsync.AttemptConnection: attemptSocket is null!");
-
                 internalArgs.RemoteEndPoint = new IPEndPoint(attemptAddress, endPoint.Port);
 
-                if (!attemptSocket.ConnectAsync(internalArgs))
+                return AttemptConnection(attemptSocket, internalArgs);
+            }
+            catch (Exception e)
+            {
+                if (e is ObjectDisposedException)
+                {
+                    NetEventSource.Fail(this, "unexpected ObjectDisposedException");
+                }
+                return e;
+            }
+        }
+
+        private Exception AttemptConnection(Socket attemptSocket, SocketAsyncEventArgs args)
+        {
+            try
+            {
+                if (attemptSocket == null)
+                {
+                    NetEventSource.Fail(null, "attemptSocket is null!");
+                }
+
+                bool pending = attemptSocket.ConnectAsync(args);
+                if (!pending)
                 {
-                    return new SocketException(internalArgs.SocketError);
+                    InternalConnectCallback(null, args);
                 }
             }
             catch (ObjectDisposedException)
             {
-                // This can happen if the user closes the socket, and is equivalent to a call 
+                // This can happen if the user closes the socket, and is equivalent to a call
                 // to CancelConnectAsync
-                return new SocketException(SocketError.OperationAborted);
+                return new SocketException((int)SocketError.OperationAborted);
             }
             catch (Exception e)
             {