Memory leak when doing fire-and-forget of Task.Delay with CancellationToken

Question

In my application I need to schedule executing of some Actions with delay. It's like setTimeout in JavaScript. Also, when app execution ends I need to cancel all scheduled executions that have not been executed yet. So I have to call Task.Delay without await and pass CancellationToken into it. But if I do so, I face with memory leak: none of CancellationTokenSource+CallbackNode will be disposed until I call Cancel and Dispose of CancellationTokenSource from which I take CancellationTokens to pass to Task.Delay.

Minimal reproducible example:

CancellationTokenSource cts = new CancellationTokenSource();
for (int i = 0; i < 1000; i++)
{
    Task.Delay(500, cts.Token).ContinueWith(_ => Console.WriteLine("Scheduled action"));
}
await Task.Delay(1000);
Console.ReadLine();

After executing this example, it leave 1000 of CancellationTokenSource+CallbackNode.

If I write cts.Cancel() after await Task.Delay(1000); leak does not appear

Why this leak happens? All Tasks was completed, so there should not be references to cts.Token. Disposing passed Task to continuation action does not help.
Also, if I await Task that schedule execution of action, leak does not appear.

Unless you run a Task, it will not execute. What you are doing it configuring a task to do something, but not actually running it. You could capture the tasks as you create them and then call Tasks.WhenAll(taskList) which WILL schedule them. — Neil, Mar 12 '22 at 10:14
Does it make any difference if you add `GC.Collect()` before the `Console.ReadLine()`? — Theodor Zoulias, Mar 12 '22 at 10:15
@Neil the `Task.Delay` creates a hot task. Awaiting the task is certainly not a requirement for that task getting scheduled. You might confusing it with the [`AsyncLazy`](https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.threading.asynclazy-1) type. — Theodor Zoulias, Mar 12 '22 at 10:23
@TheodorZoulias not 100% convinced, but OK. IMO you should never fire-and-forget a task, because you have no idea it's completed, or if an exception was thrown, and you don't know if it's been disposed. It's OK to capture the task created and look later on, but I would reject any code that did what the OP's code does. If your class creates an object, it's up to your class to dispose of it too. — Neil, Mar 12 '22 at 10:34
@Neil about fire-and-forgetting tasks not being a good idea, I agree. — Theodor Zoulias, Mar 12 '22 at 10:39
@TheodorZoulias , `GC.Collect()` before the `Console.ReadLine()` does not make any difference. — Leemellaret, Mar 12 '22 at 13:04

Kevin Gosse · Accepted Answer · 2022-03-12T10:46:26.047

5

I was a bit surprised, but it looks like this is on purpose. When a registration is cancelled, CancellationTokenSource still keeps the instance of CancellationTokenSource+CallbackNode in a free-list to reuse it for the next callback. This is an opiniated optimization, that can backfire in your scenario.

To illustrate this, try running your test twice in a row:

var tasks = new Task[1000];

for (int i = 0; i < 1000; i++)
{
    tasks[i] = Task.Delay(500, cts.Token).ContinueWith(_ => Console.WriteLine("Scheduled action"));
}
await Task.WhenAll(tasks);

for (int i = 0; i < 1000; i++)
{
    tasks[i] = Task.Delay(500, cts.Token).ContinueWith(_ => Console.WriteLine("Scheduled action"));
}
await Task.WhenAll(tasks);

Console.ReadLine()

You will see that there are still 1000 instances of CancellationTokenSource+CallbackNode, despite registering 2000 callbacks in total. That's because the second iteration reused the nodes created during the first one.

Not much you can do about this, I believe this is by design. In any case, the amount of memory should be mostly negligible (at most x instances, where x is the number of simultaneously registered callbacks).

edited Mar 12 '22 at 10:46

answered Mar 12 '22 at 10:35

Kevin Gosse

38,392
3
78
94

1

In case you're curious, the freelist was introduced in .NET Core 2.1.0: https://github.com/dotnet/coreclr/pull/12819 – Kevin Gosse Mar 12 '22 at 10:43
1

This controversial optimization reminds me of a similar one in the Channels library: [Channels with CancellationTokenSource with timeout memory leak after dispose](https://stackoverflow.com/questions/67573683/channels-with-cancellationtokensource-with-timeout-memory-leak-after-dispose). Depending on the angle from which you are looking it, it can be either a brilliant idea or a flaw. If you report this issue on GitHub, chances are that it will be dismissed as "by design". – Theodor Zoulias Mar 12 '22 at 11:18
*"Not much you can do about this"* why can't you just dispose `cts`? – Charlieface Mar 13 '22 at 06:22
@Charlieface Of course. I just meant "no way to reclaim the memory without disposing the cancellation token" (which might not be an option if for instance the token is tied to a long-running operation) – Kevin Gosse Mar 13 '22 at 10:50

Memory leak when doing fire-and-forget of Task.Delay with CancellationToken

1 Answers1