Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Socket.DisconnectAsync and related changes #46903

Closed
wants to merge 13 commits into from

Conversation

geoffkizer
Copy link
Contributor

(1) Harmonize Disconnect tests using SocketTestHelper and bring them back to inner loop, and add some additional tests
(2) Implement new Task-based DisconnectAsync API using SAEA under the covers
(3) Reimplement existing BeginDisconnect/EndDisconnect APIs on top of Task-based async DisconnectAsync API

Fixes #1608
Contributes to #43935
Contributes to #43845

@antonfirsov @wfurt @scalablecory @stephentoub @dotnet/ncl

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jan 13, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

(1) Harmonize Disconnect tests using SocketTestHelper and bring them back to inner loop, and add some additional tests
(2) Implement new Task-based DisconnectAsync API using SAEA under the covers
(3) Reimplement existing BeginDisconnect/EndDisconnect APIs on top of Task-based async DisconnectAsync API

Fixes #1608
Contributes to #43935
Contributes to #43845

@antonfirsov @wfurt @scalablecory @stephentoub @dotnet/ncl

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Sockets, new-api-needs-documentation

Milestone: -

@geoffkizer geoffkizer added this to the 6.0.0 milestone Jan 13, 2021
Comment on lines +35 to +36
// Note that the new connect operation must be asynchronous
// (why? I'm not sure, but that's the way it works currently)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is expected behavior, I guess it should be documented. Do we know someone who understands this better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. We seem to be enforcing this limitation ourselves, though I don't know if it's because DisconnectEx does not support it, or for some other (possibly legacy and no longer relevant) reason.

I skimmed the DisconnectEx docs on MSDN and didn't see anything that seemed relevant here.

I'm inclined to leave this as is currently and file a separate issue to investigate further. Sound reasonable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, since the question is not strictly related to the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an issue here: #47206


[Theory]
[InlineData(true)]
[InlineData(false)]
[OuterLoop("https://github.com/dotnet/runtime/issues/18406")]
public void Disconnect_Success(bool reuseSocket)
public async Task Disconnect_Success(bool reuseSocket)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also need to investigate / test Disconnect behavior during pending operations.

I tried to add a test for that, and discovered an inconsistency across OS-es:

        [Theory]
        [InlineData(true)]
        [InlineData(false)]
        public async Task Disconnect_DuringPendingReceive_Success(bool reuseSocket)
        {
            IPEndPoint loopback = new IPEndPoint(IPAddress.Loopback, 0);
            using (var server1 = SocketTestServer.SocketTestServerFactory(SocketImplementationType.Async, loopback))
            {
                using (Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
                {
                    await ConnectAsync(client, server1.EndPoint);
                    Assert.True(client.Connected);

                    var pendingReceive = client.ReceiveAsync(new byte[32], SocketFlags.None);

                    await DisconnectAsync(client, reuseSocket);
                    Assert.False(client.Connected);

                    // On Windows we receive 0 bytes:
                    int bytesReceived = await pendingReceive;
                    Assert.Equal(0, bytesReceived);

                    // On Linux, SocketException will be thrown:
                    // await Assert.ThrowsAsync<SocketException>(() => pendingReceive); // "operation cancelled"
                }
            }
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. How does that compare to doing Shutdown or Close with a pending operation?

BTW, we have existing tests for Shutdown/Close with pending operations, right? Do you know where these are?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only related tests I was able to find are in SendReceive: Receive0ByteReturns_WhenPeerDisconnects, TcpReceiveSendGetsCanceledByDispose, behavior seems to differ.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought @tmds added some tests related to shutdown and/or close with pending operations, back when he made a big change to cleanup/improve the close logic in general. @tmds?

Copy link
Member

@tmds tmds Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are CanceledByDispose tests for Receive, Send, Accept, Connect and SendFile. You can search CanceledByDispose to find them. You can search CanceledByDispose to find them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's what I was thinking of. I'll look at those.

@@ -1164,37 +1175,30 @@ private unsafe SocketError FinishOperationConnect()
private void CompleteCore()
{
_strongThisRef.Value = null; // null out this reference from the overlapped so this isn't kept alive artificially
if (_singleBufferHandleState != SingleBufferHandleState.None)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You decided that making CompleteCore inlineable wasn't worthwhile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I just haven't fixed it yet

new AwaitableSocketAsyncEventArgs(this, isReceiveForCaching: false);

saea.DisconnectReuseSocket = reuseSocket;
saea.WrapExceptionsForNetworkStream = false;
Copy link
Member

@stephentoub stephentoub Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to comment that this line shouldn't be necessary, but I see from the implementation that it is, which makes me think we should also be setting this here:


It's super rare that it would be an issue, as it would require disconnecting and then reconnecting the same socket, end up using the same args instance that was previously used via NetworkStream, and hit a failure condition as part of the connect... and even then the impact would just be the wrong exception message. But, still, probably worth fixing.

{
Debug.Assert(Volatile.Read(ref _continuation) == null, $"Expected null continuation to indicate reserved for use");

// TODO: Cancellation token handling
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still a "todo"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really

@@ -93,7 +93,7 @@ internal unsafe SocketError DoOperationConnect(Socket socket, SafeSocketHandle h
return socketError;
}

internal SocketError DoOperationDisconnect(Socket socket, SafeSocketHandle handle)
internal SocketError DoOperationDisconnect(Socket socket, SafeSocketHandle handle, CancellationToken cancellationToken)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we check for cancellation anywhere up until this point? If not, since the token isn't used by the implementation, it might be worth polling the token once here and returning an appropriate cancellation error before proceeding to do the disconnect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a test for this -- Disconnect_Precanceled_ThrowsOperationCanceledException -- and it's passing, so we must be checking the cancellation token somewhere in all this.

@tmds
Copy link
Member

tmds commented Jan 20, 2021

@geoffkizer I don't mind this API getting added, though I'm not sure when it should be used.

Linux doesn't have the concept of async disconnect of a socket.
If you want to know your peer has received (and processed!) something, the application protocol should send a confirmation message.
What is DisconnectAsync meant for?

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One recommendation, otherwise looks good.

Bit worried about the lack of tests for the in-flight cancellation case on Windows, but I guess there is no way to implement them in a reliable way.

if (_singleBufferHandleState == SingleBufferHandleState.InProcess)
{
RegisterToCancelPendingIO(overlapped, cancellationToken); // must happen before we change state to Set to avoid race conditions
_singleBufferHandleState = SingleBufferHandleState.Set;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not an issue, but that this will lead to _singleBufferHandle.Dispose() in Complete.

Also note that after these changes, the only semantical difference between ProcessIOCPResult and ProcessIOCPResultWithSingleBufferHandle is the pinning of the handle. Isn't it worth to dedupe that code at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is. I am trying to clean this up, but it's a little tricky at points...

@geoffkizer
Copy link
Contributor Author

@tmds This is really just an API completion issue. We have sync Disconnect, we have Begin/End disconnect, we have DisconnectAsync(SocketAsyncEventArgs). This PR is just to add a Task-based version.

I expect it to be used very rarely.

@geoffkizer
Copy link
Contributor Author

Closing this issue for now since I am not actively working on it. Waiting to figure out how to manage the cancellation state properly and consistently across operations.

@geoffkizer geoffkizer closed this Feb 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement DisconnectAsync method in SocketTaskExtensions
4 participants