[release/2.1] Fix CancellationTokenRegistration.Token after CTS.Dispose

[stephentoub]

CTR.Token should never throw, but it's currently throwing an ObjectDisposedException if the associated CancellationTokenSource has been disposed.

Ports #21394 to release/2.1
Fixes https://github.com/dotnet/corefx/issues/33844

[stephentoub]

Description

We 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 Impact

e.g.
Exceptions on background threads that crash the process when performing operations like:

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.

Risk

Minimal. The new code is effectively the same as the old, except avoiding a ThrowIfDisposed call.

[stephentoub]

Closing for now re: #21417 (comment)

[danmoseley]

@RussKeldorph so we should remove the label I guess?

[GalaxyNetworks]

From: https://dotnet.microsoft.com/platform/support/policy/dotnet-core
.NET Core 2.1 | May 30, 2018 | 2.1.18 | May 12, 2020 | LTS | August 21, 2021

I thought that 2.1 was LTS? Could you please give a valid reason for why a simple change like this has been abandoned?

I seriously hope you'll stabilize development with .NET 5. Ignoring issues like this on an LTS version, while merging the solution to 2.2, does NOT make it easy for larger companies to develop software on .NET Core!

Right now we have servers crashing due to Kestrel trying to send simple files like favicon and then... BAAM! ObjectDisposedException...

I'm surprised that no one else have commented and complained in all these years. Maybe they just upgraded to 2.2, we are however stuck on 2.1 while we upgrade to 3.1.

When complete, everything needs to be tested. Especially after the breaking change to EF Core. Runtime exceptions are never fun, especially when you could have thrown then at compile time!
At least we have integration tests...