Fix SocketAsyncEventArgs' handling of ExecutionContext#30712
Fix SocketAsyncEventArgs' handling of ExecutionContext#30712stephentoub merged 1 commit intodotnet:masterfrom
Conversation
SocketAsyncEventArgs has a few issues with ExecutionContext, presumably stemming from the fact that capturing ExecutionContext in .NET Framework is not a cheap operation. As a result, when this code was written, it was optimized for avoiding calls to ExecutionContext.Capture. The SAEA tries to hold onto a captured ExecutionContext for as long as possible, only re-capturing when either the SAEA is used with a different socket instance or when an event handler is changed. That has several problems, though. First, it largely violates the purpose of ExecutionContext, which is to flow information from the point where the async operation begins to the continuation/callback, but if the context is only being captured when the Socket or handler is changed, then the context isn't actually tied to the location where the async operation begins, and that means that data like that in an AsyncLocal doesn't properly flow across the async point. Second, it means that the SocketAsyncEventArgs (the whole purpose of which is to cache it) can end up keeping state in an ExecutionContext alive well beyond when it should be kept alive, because the SocketAsyncEventArgs is holding onto the ExecutionContext instance until either the Socket or handler is changed. This commit fixes this behavior. Since ExecutionContext.Capture in .NET Core is relatively cheap (no allocation, primarily just a ThreadStatic access), we now just always capture the context when starting an operation, and then clear it out when completing the operation.
|
Any chance that this will be back-ported to release/2.1? Or release/2.2 at the very least? |
I don't think this would make the bar for 2.1, though @danmosemsft can correct me if I'm wrong. @karelz, can you comment on 2.2? |
|
What is the impact of the bug? Which scenarios are impacted? How bad is it? How common are the scenarios? |
If there are objects in AsyncLocals when you make an http request that requires opening a new http connection, those objects may be kept alive until the next time that same pooled state in the connection manager is reused to open another connection. Fully addressing it also requires dotnet/coreclr#18670. |
|
In our case, it caused a pretty severe memory leak that crashed our servers since we are opening connections to a variety of sources all the time. However, for now we are working around it using https://github.com/dotnet/corefx/issues/30670#issuecomment-400776344. |
|
Another work around is to disable the SocketsHttpHandler using environment variables but that is not very desirable. |
| FinishOperationSyncFailure(socketError, bytesTransferred, flags); | ||
|
|
||
| if (_context == null) | ||
| if (context == null) |
There was a problem hiding this comment.
Since this pattern is repeated a couple times, couldn't we encapsulate it into something like "InvokeCompletion"?
And couldn't we move the clearing of the execution context there?
There was a problem hiding this comment.
And couldn't we move the clearing of the execution context there?
We need to clear it even on synchronous completion, in which case we're not invoking a callback.
This is surprising to me. We only cache Environment.ProcessorCount event arg instances, so at worst I'd expect that many ExecutionContexts to be kept alive beyond when they could otherwise be collected. Can you elaborate on how this resulted in such a severe leak? |
|
Requested backport to 2.2 https://github.com/dotnet/corefx/issues/31969#issuecomment-416197153 |
…#30712) SocketAsyncEventArgs has a few issues with ExecutionContext, presumably stemming from the fact that capturing ExecutionContext in .NET Framework is not a cheap operation. As a result, when this code was written, it was optimized for avoiding calls to ExecutionContext.Capture. The SAEA tries to hold onto a captured ExecutionContext for as long as possible, only re-capturing when either the SAEA is used with a different socket instance or when an event handler is changed. That has several problems, though. First, it largely violates the purpose of ExecutionContext, which is to flow information from the point where the async operation begins to the continuation/callback, but if the context is only being captured when the Socket or handler is changed, then the context isn't actually tied to the location where the async operation begins, and that means that data like that in an AsyncLocal doesn't properly flow across the async point. Second, it means that the SocketAsyncEventArgs (the whole purpose of which is to cache it) can end up keeping state in an ExecutionContext alive well beyond when it should be kept alive, because the SocketAsyncEventArgs is holding onto the ExecutionContext instance until either the Socket or handler is changed. This commit fixes this behavior. Since ExecutionContext.Capture in .NET Core is relatively cheap (no allocation, primarily just a ThreadStatic access), we now just always capture the context when starting an operation, and then clear it out when completing the operation. Commit migrated from dotnet/corefx@851a53b
SocketAsyncEventArgs has a few issues with ExecutionContext, presumably stemming from the fact that capturing ExecutionContext in .NET Framework is not a cheap operation. As a result, when this code was written, it was optimized for avoiding calls to ExecutionContext.Capture. The SAEA tries to hold onto a captured ExecutionContext for as long as possible, only re-capturing when either the SAEA is used with a different socket instance or when an event handler is changed. That has several problems, though. First, it largely violates the purpose of ExecutionContext, which is to flow information from the point where the async operation begins to the continuation/callback, but if the context is only being captured when the Socket or handler is changed, then the context isn't actually tied to the location where the async operation begins, and that means that data like that in an AsyncLocal doesn't properly flow across the async point. Second, it means that the SocketAsyncEventArgs (the whole purpose of which is to cache it) can end up keeping state in an ExecutionContext alive well beyond when it should be kept alive, because the SocketAsyncEventArgs is holding onto the ExecutionContext instance until either the Socket or handler is changed.
This commit fixes this behavior. Since ExecutionContext.Capture in .NET Core is relatively cheap (no allocation, primarily just a ThreadStatic access), we now just always capture the context when starting an operation, and then clear it out when completing the operation.
Fixes #30670
Depends on dotnet/coreclr#18670 (the new SocketsHttpHandler test won't pass until that's merged and consumed into corefx)
cc: @geoffkizer, @davidsh, @kouvel