Forward CancellationToken in Task-based Socket.ConnectAsync

[antonfirsov]

Fixes #75889

Unlike other Task-based Socket operations, ConnectAsync does not forward the CancelationToken to the PAL, instead it creates a CTR that uses CancelConnectAsync which is implemented by simply disposing the socket. This can lead to a race between Connect completion and socket disposal resulting in an invalid socket state where a disposed or non-connected socket is returned from a seemingly succesful connect attempt.

I couldn't find a way to implement a deterministic test for the scenario in #75889, but according to my local testing, the issue is gone with the changes.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[Copilot]

Pull Request Overview

This PR forwards the CancellationToken in Task-based Socket.ConnectAsync calls to address a race condition between connect completion and socket disposal.

• The DnsConnectAsync method now accepts a CancellationToken and conditionally overrides it based on existing state.
• The Windows, Unix, and Tasks implementations of ConnectAsync and related methods are updated to propagate and utilize the CancellationToken.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs	Added a CancellationToken parameter in DnsConnectAsync and updated logic to conditionally use _multipleConnectCancellation.
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs	Added CancellationToken parameters in DoOperationConnect/DoOperationConnectEx and forwards it to ProcessIOCPResult.
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Unix.cs	Propagated CancellationToken in DoOperationConnect and DoOperationConnectEx operations.
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs	Updated ConnectAsync to forward a CancellationToken to the _sendQueue.
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs	Modified ConnectAsync overloads to include and propagate a CancellationToken.
src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs	Updated task-based ConnectAsync methods to directly use the CancellationToken.

Comments suppressed due to low confidence (3)

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs:687

• In DnsConnectAsync, the method overrides the provided cancellationToken with _multipleConnectCancellation.Token when available. Please ensure this design is well-documented and that the intended cancellation behavior is maintained.

            if (_multipleConnectCancellation is not null)

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.Windows.cs:289

• The cancellationToken parameter is forwarded to ProcessIOCPResult, yet SocketPal.Connect is inherently synchronous. Verify that cancellation is either properly supported or that its behavior is clearly documented here.

        internal SocketError DoOperationConnect(SafeSocketHandle handle, CancellationToken cancellationToken)

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs:1187

• The updated ConnectAsync now directly utilizes the CancellationToken and stores it in _cancellationToken. Ensure that this change maintains the expected cancellation semantics and does not introduce any race conditions.

            public ValueTask ConnectAsync(Socket socket, CancellationToken cancellationToken)

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[wfurt]

generally looks good. Can we add test that attempt to cancel? I know it is tricky business but it would be good at least to exercise the path.

[antonfirsov]

@wfurt we do have tests for cancellation. Unfortunately, they are Windows specific, since a connection attempt to an unroutable IP (or a not-listening socket) leads to an immediate failure on Unix. We would need to use tc to emulate delay, which is not doable without dockerized testing.

If you mean a regression test for #75889, unfortunately I don't see a way to reproduce it reliably since it's timing-critical.

[wfurt]

LGTM

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

outerloop test failures are unrelated

[antonfirsov]

/ba-g android build failure is not related to the change