Uh oh!
There was an error while loading. Please reload this page.
[Draft] Idea for detection of synchronous completions for APM over Task in sockets - #51693
[Draft] Idea for detection of synchronous completions for APM over Task in sockets#51693antonfirsov wants to merge 2 commits into
Conversation
ghost
commented
Apr 22, 2021
Tagging subscribers to this area: @dotnet/ncl |
| t = saea.SocketError == SocketError.Success ? | ||
| Task.FromResult(saea.AcceptSocket!) : | ||
| Task.FromException<Socket>(GetException(saea.SocketError)); | ||
| Task.FromResult(saea.AcceptSocket!) : TaskToApm.GetSynchronousExceptionTask<Socket>(GetException(saea.SocketError)); |
There was a problem hiding this comment.
Note this is making the failure path more expensive for everyone, not just code using the APM methods.
| else | ||
| { | ||
| buffer = new byte[receiveSize]; | ||
| bytesReceived = await s.ReceiveAsync(buffer, SocketFlags.None).ConfigureAwait(false); |
There was a problem hiding this comment.
I believe it's possible the AcceptEx could previously have completed both the accept/receive synchronously, with errors from the receive aspect propagated synchronously. Now that receive is a separate operation inside of an async method, the workaround this PR is trying to utilize doesn't achieve its goal of making everything that could have failed synchronously before still fail synchronously.
| return TaskToApm.Begin(SendAsync(new ReadOnlyMemory<byte>(buffer, offset, size), socketFlags, default).AsTask(), callback, state); | ||
| var task = SendAsync(new ReadOnlyMemory<byte>(buffer, offset, size), socketFlags, default).AsTask(); | ||
| task.ThrowIfFailedSynchronously(); |
There was a problem hiding this comment.
This only addresses the concern for SendAsync because it's a single operation. It doesn't address cases where we're now using an async method to compose over multiple operations, like BeginConnect(IPAddress[], ...). It used to be that this would call BeginXx on the first address in the array and propagate that exception synchronously if one occurred. But we've cleaned up all of that code, and now all of the connection attempts to each IPAddress in the array are part of a loop in an async method. This proposed workaround doesn't help with such cases.
davidfowl
commented
Apr 22, 2021
Just for my education: Why put this much effort in trying to keep the APM code compatible? |
@davidfowl TBH it's rather part of my (still ongoing) education process about how we do things 😄 Personally, what I hate the most about the APM changes is, that no one seems to really care that we introduce deviations from API docs silently. I hoped that this can be avoided easily in code, which is seemingly not the case, so I'm giving up on compatibility and will create a work item in the API doc repo instead. |
This is an untested demonstration for my idea to resolve#47905 for
BeginSendandBeginAccept, built on top of #51212./cc @geoffkizer@stephentoub