-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[release/2.2] Fix CancellationTokenRegistration.Token after CTS.Dispose #21417
Conversation
CTR.Token should never throw, but it's currently throwing an ObjectDisposedException if the associated CancellationTokenSource has been disposed.
DescriptionWe added the CancellationTokenRegistration.Token property in .NET Core 2.1. Since the CTR already knows with what token it's associated, allowing you to get the token from the CTR saves you from also needing to separately store the token, which lets you improve memory utilization by not carrying it around effectively twice. We took advantage of that, for example, in FileStream's Read/WriteAsync operations, to decrease the size of the object allocated for those operations. However, the CTR.Token property is incorrectly throwing an ObjectDisposedException if the associated CancellationTokenSource has been disposed, which introduces an ODE where there wasn't previously one (just accessing a CancellationToken you already had stored). This is causing regressions in code that created a token source, registered with its token, and then disposed the source. Customer Impacte.g. var cts = new CancellationTokenSource();
Task<int> t = fileStream.ReadAsync(..., cts.Token);
cts.Dispose();
int result = await t; ASP.NET actually has a case of this today, e.g. dotnet/aspnetcore#4447. Regression?Yes. It's not a regression in CancellationTokenRegistration.Token, which was only introduced in 2.1, but rather a regression in that it's behavior isn't matching code it was used to replace, so it's for example a regression in FileStream.Read/WriteAsync. Packaging reviewed?Change affects System.Private.CoreLib only. RiskMinimal. The new code is effectively the same as the old, except avoiding a ThrowIfDisposed call. |
Approved for 2.2.1.
We should wait for any 2.1 fix -- close the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable.
@halter73 Let's verify that this fixes the customer issue (once we have a build.) Thanks. |
@halter73, can you help validate it fixes the ASP.NET issue? |
@stephentoub Sure. Are there any build artifacts I can use to test this PR or do I have to build myself? |
Thanks, Stephen. Yes, e.g. on Windows: |
I used the "checked" x64 artifacts from https://ci.dot.net/job/dotnet_coreclr/job/release_2.2/job/x64_checked_windows_nt_innerloop_prtest/231/artifact/bin/Product/Windows_NT.x64.Checked/. This was easier for me since I already have the x64 SDKs installed on my workstation. I simply dropped those artifacts into $DOTNET_HOME\shared\Microsoft.NETCore.App\2.2.0\ and tested against that. My repro app which consistently crashes with an ODE in less than a minute on the 2.2.0 runtime, didn't crash at all in 10 minutes of testing when the runtime was patched with the artifacts from this PR. @stephentoub Is there a better way of using these artifacts for testing? If you want to try the repro yourself, it's super simple: just clone
|
Very helpful, @halter73, thank you. I generally do something similar, patching my shared framework. I did the same here, using a locally-built release build, and no longer see the crashes that I generally saw with your repro very quickly, often within 10 seconds. |
CTR.Token should never throw, but it's currently throwing an ObjectDisposedException if the associated CancellationTokenSource has been disposed.
Ports #21394 to release/2.2
Fixes https://github.com/dotnet/corefx/issues/33844