This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Fix order of native overlapped freeing in SocketAsyncEventArgs on Windows #27728
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…dows SocketAsyncEventArgs tracks its disposal, and if it's disposed while an async operation is in flight, it delays the actual disposal until the operation completes. That disposal includes disposing of the PreAllocatedOverlapped used for async operations. When an operation is initiated, if the operation completes synchronously, then the NativeOverlapped associated with that PreAllocatedOverlapped is freed in a finally block once the operation completes synchronously. But if during the operation the SAEA is disposed of, then the PreAllocatedOverlapped can get disposed of before the finally block. The fix is to ensure we clean up the NativeOverlapped before completing the operation. This was already handled correctly when an operation completed asynchronously, it just needed to be fixed for when it completes synchronously. As a happy side-effect, fixing this also cleans up the call sites a bit. (This was highlighted by an assert once we switched Socket.WriteAsync to return a ValueTask instead of a Task, because the way it was done means that when the Socket is disposed, the associated SocketAsyncEventArgs will be disposed, and SocketsHttpHandler is aggressive about disposing of the Socket as a way to enable cancellation, so the assert was triggering not-infrequently when running the System.Net.Http tests, especially in CI.)
geoffkizer
reviewed
Mar 5, 2018
| @@ -186,19 +200,15 @@ internal unsafe SocketError DoOperationAccept(Socket socket, SafeCloseSocket han | |||
| out int bytesTransferred, | |||
| overlapped); | |||
|
|
|||
| socketError = ProcessIOCPResult(success, bytesTransferred); | |||
| return socketError; | |||
| return ProcessIOCPResult(success, bytesTransferred, overlapped); | |||
There was a problem hiding this comment.
I realize this is the current pattern, but...
It seems slightly weird that we call ProcessIOCPResult inside the try/catch. ProcessIOCPResult should never throw, so it's slightly cleaner to call it outside the try. But if it did throw, seems like we'd free the NativeOverlapped twice.
Maybe not worth worrying about for this PR, just something to consider...
|
LGTM. Thanks for finding this! |
geoffkizer
approved these changes
Mar 5, 2018
|
@dotnet-bot test Outerloop Windows x64 Debug Build please |
|
@rmkerr We might want to consider porting this to NETFX if the problem is there too. |
davidsh
approved these changes
Mar 5, 2018
|
@davidsh I expect we'd get the same behavior on NETFX. I don't understand the consequences of the bug well enough to say whether or not it could be the cause of any socket issues we've seen before, but it definitely seems plausible. Either way, it's worth looking into! |
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
…dows (dotnet/corefx#27728) SocketAsyncEventArgs tracks its disposal, and if it's disposed while an async operation is in flight, it delays the actual disposal until the operation completes. That disposal includes disposing of the PreAllocatedOverlapped used for async operations. When an operation is initiated, if the operation completes synchronously, then the NativeOverlapped associated with that PreAllocatedOverlapped is freed in a finally block once the operation completes synchronously. But if during the operation the SAEA is disposed of, then the PreAllocatedOverlapped can get disposed of before the finally block. The fix is to ensure we clean up the NativeOverlapped before completing the operation. This was already handled correctly when an operation completed asynchronously, it just needed to be fixed for when it completes synchronously. As a happy side-effect, fixing this also cleans up the call sites a bit. (This was highlighted by an assert once we switched Socket.WriteAsync to return a ValueTask instead of a Task, because the way it was done means that when the Socket is disposed, the associated SocketAsyncEventArgs will be disposed, and SocketsHttpHandler is aggressive about disposing of the Socket as a way to enable cancellation, so the assert was triggering not-infrequently when running the System.Net.Http tests, especially in CI.) Commit migrated from dotnet/corefx@f5e9b75
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SocketAsyncEventArgs tracks its disposal, and if it's disposed while an async operation is in flight, it delays the actual disposal until the operation completes. That disposal includes disposing of the PreAllocatedOverlapped used for async operations. When an operation is initiated, if the operation completes synchronously, then the NativeOverlapped associated with that PreAllocatedOverlapped is freed in a finally block once the operation completes synchronously. But if during the operation the SAEA is disposed of, then the PreAllocatedOverlapped can get disposed of before the finally block.
The fix is to ensure we clean up the NativeOverlapped before completing the operation. This was already handled correctly when an operation completed asynchronously, it just needed to be fixed for when it completes synchronously. As a happy side-effect, fixing this also cleans up the call sites a bit.
(This was highlighted by an assert once we switched Socket.WriteAsync to return a ValueTask instead of a Task, because the way it was done means that when the Socket is disposed, the associated SocketAsyncEventArgs will be disposed, and SocketsHttpHandler is aggressive about disposing of the Socket as a way to enable cancellation, so the assert was triggering not-infrequently when running the System.Net.Http tests, especially in CI.)
Fixes https://github.com/dotnet/corefx/issues/27721
cc: @geoffkizer, @davidsh