SemaphoreSlim memory leak

[alexclarkmusa]

Description

Performing a simple loop & wait while asynchronously awaiting either a SemaphoreSlim event or a task (via Task.WhenAny) seems to cause an ongoing increase in the number of "SemaphoreSlim+TaskNode" instances on the heap when examined via the Managed Memory snapshot (Visual Studio Diagnostic Tools).

Sample code shows a fairly common example of a service that might be watching ~100 different resources, polling every 5 seconds for new data/files/events. It either waits for its configured polling interval (hardcoded here to 5 seconds) OR until the SemaphoreSlim gets released (an interrupt).

Reproduction Steps

internal class Program
{
    private static readonly SemaphoreSlim _interrupter = new(0, 1);
    private static readonly CancellationToken _cancellationToken = CancellationToken.None;

    static async Task Main()
    {
        var procs = Enumerable.Range(1, 100).Select(i => Process()).ToList();
        await Task.WhenAll(procs);
    }

    private static async Task Process()
    {
        while (true)
        {
            await Interruption(TimeSpan.FromMilliseconds(5000));
            // look for new data here
        }
    }

    private static async Task Interruption(TimeSpan delay)
    {
        var delayTask = Task.Delay(delay, _cancellationToken);
        var interruptTask = _interrupter.WaitAsync(_cancellationToken);

        // Wait until either the delay has expired or we got interrupted
        await Task.WhenAny(interruptTask, delayTask);
    }
}

Expected behavior

That the GC would collect these instances. In the code, it is not obvious/intuitive at all that this would result in a memory leak.

Actual behavior

After 5s runtime:

After 30s:

After 60s: (comparing to first snapshot)

After 5 mins runtime: (comparing to first snapshot)

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

[stephentoub]

From SemaphoreSlim's perspective, you've asked for a resource, it's given you a Task it'll complete when that resource is transferred to you, and it's dutifully doing that. You're never canceling the wait, so the SemaphoreSlim is keeping track of it so that when there's a resource available, it'll complete that Task. If you don't want the wait to be active anymore, you should cancel it.

(As an aside, you should be doing the same with the Task.Delay. In your example, it's always firing, but typically when Task.Delay is used in this way, timeout is the rarity rather than the expected case, and if you don't cancel the Delay, you'll be leaking a timer until it actually fires.)

[dotnet-policy-service]

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

[alexclarkmusa]

So just to confirm (because I couldn't find any remarks indicating this on the docs for Task.Delay) - are you saying that a Task.Delay should also be cancelled if it did not run to completion? i.e. if I were to await the task returned from Task.Delay(5000) with a timeout of 4000ms, causing it to time out rather than complete its delay, do I need to cancel that task or risk a memory leak?

This (appears to have) fixed the leak in my code:

 private async Task Interruption(TimeSpan delay)
 {
     await _interrupter.WaitAsync(delay, _cancellationToken);
 }

... am I right in assuming this is not returning a resource, or should I be actually storing the reference to the returned task and canceling it if it timed out?

(Apologies for requiring some hand-holding on this, but the docs do not make this clear!)

[stephentoub]

So just to confirm (because I couldn't find any remarks indicating this on the docs for Task.Delay) - are you saying that a Task.Delay should also be cancelled if it did not run to completion?

Yes

This (appears to have) fixed the leak in my code

Yes. Now when the timeout used implicitly by WaitAsync fires, it'll effectively cancel the wait. That's in contrast to the previous example where the timeout was handled separately, such that the wait operation had no knowledge of it.

am I right in assuming this is not returning a resource

If the delay timeout or the cancellation token trigger before the wait is successful, then the semaphore's count will not be changed, so there's nothing you need to do to release the count back to the semaphore. If instead the wait completes successfully (doesn't throw timeout / cancellation exceptions), then a count from the semaphore has been transferred to you, and if you want that released back to the semaphore, you'll need to do so.

[alexclarkmusa]

Okay, this makes sense to me. The original code was using a Task.Delay in combination with the async wait on the SemaphoreSlim so I assume the runtime was still thinking that the SemaphoreSlim might eventually release a slot, regardless of the fact that the delay task had already completed.
In my amended code I'm waiting for the semaphore up to a maximum of the 'timeout' amount (delay), and once that timeout is reached the runtime knows there's no need to continue waiting on the semaphore. So at that point it can... Let It Go.

Assuming my understanding of this is correct - thank you for helping me with this!

[stephentoub]

the SemaphoreSlim might eventually release a slot, regardless of the fact that the delay task had already completed.

Right, the two operations were completely unrelated.

I'm waiting for the semaphore up to a maximum of the 'timeout' amount (delay), and once that timeout is reached the runtime knows there's no need to continue waiting on the semaphore. So at that point it can... Let It Go.

Right.

[stephentoub]

thank you for helping me with this!

No problem.