Make LinkedCancellationTokenSource register itself weakly

[TimLovellSmith]

This should in theory fix an annoying usability issue that a LinkedCancelationTokenSource becomes a memory leak by default, any time you forget to call Dispose on it.

[ghost]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This fixes an annoying usability issue that a LinkedCancelationTokenSource becomes a memory leak by default, any time you forget to call Dispose on it.

Author:	TimLovellSmith
Assignees:	-
Labels:	area-System.Threading, community-contribution
Milestone:	-

[Wraith2]

This change would add allocations for all users and add no functionality for users using linked cancellation tokens correctly. That doesn't seem like a good trade for making poor code work better.

[TimLovellSmith]

BTW, its pretty great to my mind, if 'poor' code (blame the user mentality) can work better, since the 'works worse' scenario is memory leaks --
in what I think is supposed an easy to use, automatically garbage collected language.

Just my opinion of course.

[Wraith2]

I've had a similar that I fixed issue in dotnet/SqlClient#1818 and it was user error in that case too. Managing the lifetime is the responsibility of the author. Working with async correctly is complicated and requires a lot of mental work but providing a simpler api that attempts to manage that complexity without author interaction will incur perf overhead that can't be mitigated by people consuming that api.

This change will introduce allocations that can't be managed away by users and it's something that be hit by a lot of people wanting fast async code. I'm not attempting to "burn" I just don't think it's a good trade off in the BCL.

[TimLovellSmith]

This change will introduce allocations that can't be managed away by users and it's something that be hit by a lot of people wanting fast async code. I'm not attempting to "burn" I just don't think it's a good trade off in the BCL.

If people want a dangerous API with no perf overhead, so they can allocate only one object per linked cancellation token source, instead of two, maybe that should be provided also?

It could be called 'DangerousLinkedCancellationTokenSource', since forgetting to dispose it is kind of dangerous.

[stephentoub]

Performance aside, this is also problematic functionally and is not a change we could take. There are real cases where folks register with a cancellation token to be notified when cancellation is requested and where the linked token source isn't otherwise rooted; this will break those. For example, this test can fail with this change:

using System.Runtime.CompilerServices;

var cts = new CancellationTokenSource();
Queue(cts.Token);

Thread.Sleep(1000);
GC.Collect();
GC.WaitForPendingFinalizers();

cts.Cancel();
Console.WriteLine("Done");
Console.ReadLine();

[MethodImpl(MethodImplOptions.NoInlining)]
static void Queue(CancellationToken cancellationToken)
{
    Task.Run(async () =>
    {
        using var linked = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
        var tcs = new TaskCompletionSource();
        linked.Token.Register(tcs.SetResult);
        await tcs.Task;
        Console.WriteLine("Invoked");
    });
}

Before the change, it'll reliably print out "Invoked". After the change, there's a really good chance it won't.

Thanks for trying to improve things, though.

[TimLovellSmith]

@stephentoub That's a goood critique. I am not so surprised it would be a problem.

Neither register, nor WaitHandle roots the linked source. Darn, so many modalities to support.

I suppose if anyone uses those APIs on a linked token, which is perhaps less common, the weak references back along the chain could promote to strong ones. Maybe it would even be a 'fix' for the new bug. But it would kind of break the leak fix to add such interesting behavior.

Back to the drawing board!