Repository navigation
Possible CancellationTokenRegistration leak in AsyncOperation<T> #761
Description
Under profiling it appears as if BoundedChannel may leak cancellation callbacks when used in this manner:
var channel = Channel.CreateBounded<bool>(1);
while (true)
{
using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(100));
try
{
await channel.Reader.ReadAsync(cts.Token);
}
catch (OperationCanceledException)
{
}
}Firstly, is this an intended use case? We have a long-running Channel that multiple readers may attempt to read from (these readers may timeout and cancel their CancellationToken as they disconnect, the 100ms timeout with CTS is to simulate this faster).
If this is an intended use case it looks like _registration in AsyncOperation ctor is not disposed when cancellation path is taken? I see that TrySetResult() and TrySetException() both call UnregisterCancellation() but not TrySetCanceled(), though I am unsure if my understanding of this is correct.
In addition to this (or perhaps as a result of it) it seems like OperationCanceledExceptions are accumulated via ExceptionDispatchInfo and a reference back to Deque.
Activity
stephentoub commentedon Dec 11, 2019
Thanks. Are you actually observing the effects of a leak, i.e. memory grows unbounded, or is this based on code review only?
stephentoub commentedon Dec 11, 2019
Ah, I took a look at your repro and understand now why you're seeing what you're seeing.
It's not actually about unregistering from the CancellationToken. We don't need to unregister when cancellation occurs because unregistering at that point would be a nop: cancellation already occurred, effectively automatically unregistering everything at that point.
The issue you're seeing is by-design in the bounded channel, though we might be able to improve things... you can see reference to it in the comment here, and it's why this is a loop:
runtime/src/libraries/System.Threading.Channels/src/System/Threading/Channels/BoundedChannel.cs
Lines 339 to 342 in 5d4304e
The bounded channel maintains a collection of the AsyncOperation instances that have been created, and when one of them is canceled, it marks the instance as such but doesn't actually remove it from the collection (for performance / simplicity). In this case, you're creating AsyncOperations for reads; when a write then comes along and can satisfy a read, it'll pull the next item from the collection to complete it, but if it sees at that point it's already been canceled, it'll just discard it and get the next one.
In other words, in your repro, I expect you'll find that the moment you write to the bounded channel, you'll see all of those objects become unrooted and become available to the GC, because the write will end up pulling out all of the canceled objects from the queue.
So, while this behavior is by-design, it's possible we could improve things further here.
invidious9000 commentedon Dec 11, 2019
Thank you for looking at this so quickly. This came about as a result of profiling an actual application vs code review. We have also observed that if the Channel is permitted to fall out of scope or once a write occurs and queued waiters are drained then these objects are eligible for GC.
Is there any way to mitigate this behavior with a long-running Channel where there may be many reader cancellations before the next write? Perhaps something like a poison pill to drain waiters?
Clockwork-Muse commentedon Dec 11, 2019
.... As a side question, why do you have so many cancelled readers between writes?
invidious9000 commentedon Dec 11, 2019
@Clockwork-Muse We're using a Channel to aggregate writes against a transport stream (gRPC) that experiences frequent transient network issues.
The transport stream itself throws when writing from more than one thread so we have a long-running Channel that aggregates response writes for a single reader that is "attached" when transport connects. When transport connects a Task runs and attempts to ReadAsync() from the Channel and drain responses and the issue emerges when the transport connection dies (cancels) before any response writes are performed.
We're a bit uncertain if this is an acceptable use case but it seems to work well under normal conditions, it's only during long periods of transient reconnects where the cancelled-waiter nuance becomes an issue.
stephentoub commentedon Dec 12, 2019
With the shipped bits, I can't think of any, other than doing a write.
We / I didn't think this would be an issue, so we explicitly don't make any effort to remove AsyncOperations from the deques when they're canceled, and just let them get purged naturally.
The right fix for the next release is probably to make these into doubly-linked lists, and change how cancellation callbacks are handled to also remove the nodes from the relevant list.
willdean commentedon Mar 21, 2020
I have just written-up another issue (which I shall now not file), which turns out to be exactly this problem. This was based on analysing a memory dump of a real, leaking, production application which had leaked about 70000 of each of the 6-or-so objects which gets leaked for every cancellation (the relationship between the objects is complex, but it's 10s of MB leaked)
As it stands, it's clear that the BoundedChannel is not very suitable for the kind of application where there are many many timeouts for each use of the queue - clearly there are several work-arounds we could adopt (push some kind of 'null object' through the queue every so often, for example), but it's a bit sad because we had high hopes for Channel to cleanly replace a previous very complex (but working) TCS+CTS arrangement which, ironically, had taken some effort to get the leaks out of. If we have to build an additional time-based system to flush the queue somehow the cleanliness is marred.
I accept that this design decision was deliberate, but it looks very like a bug when one stumbles over it in a memory dump. Can I put in a +1 for future improvement here, please?
springy76 commentedon Jun 8, 2020
I'm randomly losing items while using a BoundedChannel (with FullMode=Wait). I most likely tracked it down to cancellation happening during
channel.Reader.ReadAsync(token)-- and the very same code is running happily for days and weeks when using an UnboundedChannel instead.Is this known/expected behavior?
stephentoub commentedon Jun 8, 2020
Without a repro, it's hard to say. Doesn't sound related to the issue you commented on, though.
springy76 commentedon Jun 8, 2020
@stephentoub I'll create a new issue for that, this issue just looked most related (Reader.ReadAsync + CancellationToken) at first place.
My repro script currently is a LINQPad script: Would you accept this or do you need a console app (which would take some time rewriting all the nice and easy LINQPad.Dump()'s)?
22 remaining items
stephentoub commentedon Aug 14, 2023
We should investigate whether there's a good way to improve this without incurring significant overhead. I believe I looked at one point and there wasn't a simple answer.
batrived commentedon Nov 7, 2023
Same issue is observed in the WaitToReadAsync. @stephentoub I see that you mentioned that a doubly-linked list for waiters could be a solution without significant overhead. Does that not hold true now? I see that SemaphoreSlim also used doubly-linked list to achieve similar scenario.
stephentoub commentedon Dec 8, 2023
Someone would need to prototype it out and measure.