Don't capture AsyncLocals into CancellationChangeToken registration#392
Don't capture AsyncLocals into CancellationChangeToken registration#392davidfowl merged 7 commits intodotnet:release/2.2from
Conversation
|
@benaadams test? |
| public IDisposable RegisterChangeCallback(Action<object> callback, object state) | ||
| { | ||
| // Don't capture the current ExecutionContext and its AsyncLocals onto the token registration causing them to live forever | ||
| bool restoreFlow = false; |
| { | ||
| // Restore the current ExecutionContext | ||
| if (restoreFlow) | ||
| ExecutionContext.RestoreFlow(); |
There was a problem hiding this comment.
ASP.NET coding standard is { } around a single line if statement.
davidfowl
left a comment
davidfowl
left a comment
There was a problem hiding this comment.
I need to do some looking and see what this might break. But this change LGTM.
We should also ask for this API to be exposed in CoreCLR https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/Threading/CancellationToken.cs#L255.
I'll file an API request for this.
| var restoreFlow = false; | ||
| try | ||
| { | ||
| if (!ExecutionContext.IsFlowSuppressed()) |
There was a problem hiding this comment.
Should this outside the if block? Doesn't really need a try-catch does it?
There was a problem hiding this comment.
is the try finally its for, the catch was there previously
| { | ||
| // AsyncLocal not set, when run on clean context | ||
| // A suppressed flow runs in current context, rather than restoring the captured context | ||
| Assert.Equal(0, ((AsyncLocal<int>) al).Value); |
There was a problem hiding this comment.
Might want to set something that verifies that this block of code was executed.
var executed = false;
...
Assert.Equal(0, ((AsyncLocal<int>) al).Value);
executed = true;
...
Assert.True(executed);
|
@davidfowl good to get this in? |
|
Yep |
ghost
locked as resolved and limited conversation to collaborators
4 participants
This causes a problem with
IHttpContextAccessoras it captures theHttpContextand everything linked to it causing a lot of memory to become rooted (as seen in https://github.com/aspnet/KestrelHttpServer/issues/2840#issuecomment-416034872)/cc @davidfowl