Calling Set() from AsyncManualResetEvent can potentially saturate the threadpool

[joproulx]

The following sample will result in what appears to be threadpool saturation:

for (int i = 0; i < 40; i++)
{
    var manualEvent = new AsyncManualResetEvent();

    Task.Run(() =>
    {
        Console.WriteLine("Set. Time={0}", DateTime.Now.ToString("hh:mm:ss"));
        manualEvent.Set();
    });
}

The output will show that the Set method is called rapidly for the first X available threads on the threadpool, and then start to slow down while the threadpool creates new threads.

On my machine, the default threadpool has 8 threads and it took 31 seconds before the last Set() could be called.

Does an async version of the Set method make sense? The following code solved my issue, but can this create any wrong side-effect that I don't see?

public Task SetAsync()
{
    bool lockTaken = false;
    object obj;
    try
    {
        Monitor.Enter(obj = this._sync, ref lockTaken);
        TaskShim.Run<bool>((Func<bool>)(() => this._tcs.TrySetResult()));
        return @this._tcs.Task;
    }
    finally
    {
        if (lockTaken)
            Monitor.Exit(obj);
    }
}

Thanks for the great library by the way!

[StephenCleary]

That's some very interesting behavior!

Internally, the Set is actually using TaskShim.Run very similarly to the way you're using it. The only difference is that AMRE.Set is then (synchronously) waiting for the task to complete before returning. This is necessary for some synchronization primitives but I don't remember if it's strictly necessary for AMRE.

I'll need to investigate this further and think about it a lot.

On a side note, the new version of AMRE is using a newer underlying implementation based on .NET 4.6 / Core types, and it does not exhibit this behavior. Feel free to upgrade if you're on .NET 4.6.

[joproulx]

Thanks for the reply Stephen. The issue is indeed caused by the fact that AMRE.Set is blocking the thread until an action pushed on threadpool is executed. But unfortunately, as my sample shows, this can cause threadpool saturation if the Set method is called rapidly by all the threadppool threads.

Based on Stephen Toub article, I am guessing that the goal of doing the logic that you do in the Set method is to avoid the synchronous continuation that would be executed as part of the Set method.

From what I see in your new version of AMRE.Set(), you call directly the TrySetResult method on the TaskCompletionSource which will avoid the threadpool saturation issue but will have the side effect of executing the continuation synchronously as part of the Set method. Am I right by saying that or is there something different in .Net 4.6 that will not behave as in 4.5?

[StephenCleary]

Yes, that is one reason. Another is that my implementations are using regular locks, and one of the cardinal rules for locks is that you can't invoke arbitrary end-user callbacks (e.g., task continuations).

The TaskCompletionSource in .NET 4.6 has a new flag RunContinuationsAsynchronously, which is used by the new version of the code.

[joproulx]

Thanks for the explanation. I will take a better look at your new implementation. As for the flag RunContinuationAsynchronously, I don't know if you were already aware of this bug, but there are some cases where the flag won't make the continuation run asynchronously in .Net 4.6 and 4.6.1.

[JeffCyr]

Note that the RunContinuationsAsynchronously flag had a bug in 4.6. It wasn't enforced if the task was passed to Task.WhenAll/WhenAny.

The issue got fixed in 4.6.1

https://github.com/dotnet/coreclr/issues/2021

---

Edit

I misread, the bug is still in .Net 4.6.1

unfortunately this was discovered too late for 4.6.1 but it will be in the next update. But it will be on coreclr before that.

[IanYates]

Yeah, shame it's still in 4.6.1. I haven't seen any news anywhere for a 4.6.2. I guess //build might give us some new toys.

[StephenCleary]

Closing this due to a fix available in the new netstandard-based coordination library.

[StephenCleary]

Follow #71 for updates on the v5 (netstandard) release.