Fix LinkedSubSource leak in Async.Choice

[Frassle]

One of our programs makes heavy use Async.Choice and we attached a memory profiler and saw huge numbers of LinkedCancellationTokens being allocated and left around by Async.Choice. One user managed to build up about 30GB of these.

[abelbraaksma]

Seriously, 30GB?!?

I still have F# in the VS IDE slowing down over time (though much better now since many performance improvements were done). Wonder if this has anything to do with that as well. Great work, hope the fix works :).

[forki]

Can you please describe the idea from a higher level? Thanks Abel Braaksma <notifications@github.com> schrieb am Do., 21. Nov. 2019, 22:48:

…

Seriously, 30GB?!? I still have F# in the VS IDE slowing down over time (though much better now since many performance improvements were done). Wonder if this has anything to do with that as well. Great work, hope the fix works :). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7892?email_source=notifications&email_token=AAAOANGSN6VBKIWVZBUN2DLQU364BA5CNFSM4JQG5ZJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE3YRGA#issuecomment-557287576>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOANERLPAJK4YDIZZXETLQU364BANCNFSM4JQG5ZJQ> .

[abelbraaksma]

@forki, I didn't make this PR, that was @Frassle. Perhaps he can give an example of how to get this leaking in practice?

For my own remark, I'm yet not 100% sure it comes from F# or some installed VS add in. But in VS 2017 I had to restart VS often after an hour's work, and coloring, or error squigglies stayed sometimes for minutes, or didn't disappear at all after fixing code. I can clearly see improvement there, auto complete works almost without delay, even in large projects (200k loc). And if errors are now stalled, I can recompile age then wait for the background services to finish,and then they disappear. Still, after several hours of work typing becomes slow again, so something is still clogging the pipes over time.

But I shouldn't clutter this thread with that. Huge improvements have been made in performance by many contributors (I cannot thank you all enough!), and once I get a good repro again, I'll create a specific issue for it.

[forki]

@abelbraaksma I don't think there is much Async.Choice used in F# tooling so that fix would probably not help with VS. I'm actually more interested in the concrete changes here

[Frassle]

I'm actually more interested in the concrete changes here

Async.Choice allocates a LinkedSubSource, which is just a CancellationTokenSource and a LinkedTokenSource. Currently these are never Disposed, and so the LinkedTokenSource is never decoupled from the parent CancellationToken in the async context. Thus these unless your clearing your top level cancellation tokens frequently (which I'd imagine is really rare) these things just continue to build up in memory and are never collected.

This change keeps track of the total number of asyncs that were started and when the last one calls back it disposes of the LinkedSubSource. Currently Choice does nothing for returning callbacks once the first results has been triggered. This matches what Async.Parallel does where once all the child asyncs are done it disposes of the LinkedSubSource it allocated.

[cartermp]

I think this looks good overall. Tagging @dsyme and @eiriktsarpalis for a spot check here

[KevinRansom]

@Frassle would weak reference be a preferable approach here? Reference counting is so prone to errors.

[TIHan]

This looks good. The reference counting approach is almost the same as Async.Parallel's implementation which uses LinkedSubSource and reference counting for its disposal.

[TIHan]

@Frassle Before your change, the objects were getting collected, but the internal os WaitHandle that CancellationTokenSource holds wasn't being released which was causing the high memory.

@KevinRansom The objects were being collected anyway; nothing is rooting the CancellationTokenSource or its internal LinkedTokenSource. The problem is the internal WaitHandle from the os that's not being released. So, we have to call Dispose on both CT sources in order to release their WaitHandles. We could add a Finalize to LinkedSubSource which calls Dispose on the CT sources to avoid a manual call to Dispose on the LinkedSubSource type; but Async.Choice is easily deterministic of when the CT sources are not going to be used anymore, so it's fine that we call Dispose manually with ref counting. But in general, ref counting is prone to errors when used in scenarios where you have to keep track of something being moved in a lot of different places outside its context.

What I wrote above were incorrect observations and assumptions.

In Async.Choice's case, the async context token prevents the collection of LinkedTokenSource, just as @Frassle said. The behavior feels subtle. If it was a free CancellationToken, i.e let ct = CancellationToken(), and passed to CreatedLinkedTokenSource, the LinkedTokenSource could be collected on its own and it will call the finalizer on the SafeHandle to release the OS handle.

[Frassle]

@TIHan our memory traces were showing lots of cancellation callbacks and linked cancellation tokens, it's not just the wait handle that's being left.

@KevinRansom as @TIHan said ref counting here matches what's done in Async.Parallel.

[TIHan]

I was incorrect. @Frassle is right, LinkedTokenSource is not getting collected because of the async context token. If a CTS does get collected and with no manual call to Dispose, it will actually release the WaitHandle as the internal WaitHandle itself is actually a SafeHandle which will call Finalize to release itself (except in CoreRT). See: https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/Runtime/InteropServices/SafeHandle.cs#L69

Here is an example of how to have high memory usage:

open System
open System.Threading

let test i ct =
    let cts1 = new CancellationTokenSource()
    let cts2 = CancellationTokenSource.CreateLinkedTokenSource(ct, cts1.Token)
    ()

[<EntryPoint>]
let main argv =
    let cts = new CancellationTokenSource()
    for i = 1 to 10000000 do
        test i cts.Token
    Console.ReadLine() |> ignore
    0

[TIHan]

Yea, a weak reference wouldn't help here as we have no choice but to manually call Dispose in order to decouple the token and CTS.

This is a great fix as well as find.

[KevinRansom]

Thanks for this.