StephenCleary / AsyncEx

A helper library for async/await.
MIT License
3.53k stars 356 forks source link

CancellationTokenTaskSource's dispose #272

Open yfital opened 1 year ago

yfital commented 1 year ago

Hey,

I'm wondering whether the current behavior is correct:

        public void Dispose()
        {
            _registration?.Dispose();
        }

When we use it (maybe our usage is different) we usually use this to hook a CT towards a WhenAll. So for instance, we'll have some sort of:

async Task MyWhenAll(tasks[], ct)
{
using var ctts = new CTTS(ct);
try 
{ 
    await await WhenAny(WhenAll(tasks), ctts.task);
}
catch
{ // handle specifics if needed
}

if ( ctts.task.iscompleted )
    return; // handle timeout

//all valid
    return;
} 

The disposal currently disposes of the registration, meaning it detaches the TCS from the CT. But the TCS is now a long-living task that is just kept in a non-finite state.

In case everything worked as expected (no exceptions), the dispose should have marked the TCS as completed, no?

That will also supported the case where someone used CTTS in a scope and disposed it, which I expected to have the TCS be marked as completed and free the WhenAll. (Not because I think someone should do it, but for consistency with the language).

To clarify, I do not think there is a leak here, it's merely an optimization/consistency look.