Deadlocking with the TPL, how to
As I mentioned, I run into a very nasty issue with the TPL. I am not sure if it is me doing things wrong, or an actual issue.
Let us look at the code, shall we?
We start with a very simple code:
1: public class AsyncEvent2: {
3: private volatile TaskCompletionSource<object> tcs = new TaskCompletionSource<object>();4:
5: public Task WaitAsync()6: {
7: return tcs.Task;8: }
9:
10: public void PulseAll()11: {
12: var taskCompletionSource = tcs;
13: tcs = new TaskCompletionSource<object>();14: taskCompletionSource.SetResult(null);15: }
16: }
This is effectively an auto reset event. All the waiters will be released when the PulseAll it called. Then we have this runner, which just execute work:
1: public class Runner : IDisposable2: {
3: private readonly ConcurrentQueue<TaskCompletionSource<object>> items =4: new ConcurrentQueue<TaskCompletionSource<object>>();5: private readonly Task<Task> _bg;6: private readonly AsyncEvent _event = new AsyncEvent();7: private volatile bool _done;8:
9: public Runner()10: {
11: _bg = Task.Factory.StartNew(() => Background());
12: }
13:
14: private async Task Background()15: {
16: while (_done == false)17: {
18: TaskCompletionSource<object> result;19: if (items.TryDequeue(out result) == false)20: {
21: await _event.WaitAsync();
22: continue;23: }
24:
25: //work here, note that we do NOT use await!26:
27: result.SetResult(null);28: }
29: }
30:
31: public Task AddWork()32: {
33: var tcs = new TaskCompletionSource<object>();34: items.Enqueue(tcs);
35:
36: _event.PulseAll();
37:
38: return tcs.Task;39: }
40:
41: public void Dispose()42: {
43: _done = true;44: _event.PulseAll();
45: _bg.Wait();
46: }
47: }
And finally, the code that causes the problem:
1: public static async Task Run()2: {
3: using (var runner = new Runner())4: {
5: await runner.AddWork();
6: }
7: }
So far, it is all pretty innocent, I think you would agree. But this cause hangs with a dead lock. Here is why:
Because tasks can share threads, we are in the Background task thread, and we are trying to wait on that background task completion.
Result, deadlock.
If I add:
1: await Task.Yield();
Because that forces this method to be completed in another thread, but that looks more like something that you add after you discover the bug, to be honest.
Comments
Hmm, have a look at TPL Dataflow - https://nuget.org/packages/Microsoft.Tpl.Dataflow. It can save you all this hard code.
I think you should use better soft language. I have use MS VB6 with lot of happy without this problems.
Ayende, we can have a reviewing of TPL Dataflow at https://nuget.org/packages/Microsoft.Tpl.Dataflow?
thanks
I do see a bug:
1) The background thread executes items.TryDequeue(out result), which returns false 2) Another thread executes AddWork, which calls PulseAll and replaces the TCS in AsyncEvent with a new one, "pulsing" the old one 3) The background thread awaits on the event, having missed the notification.
Background() should be
while (_done == false) { TaskCompletionSource<object> result; var t = _event.WaitAsync(); Thread.MemoryBarrier(); if (items.TryDequeue(out result) == false) // Has a memory barrier, so no reorder will happen { await t; continue; }
result.SetResult(null); }
Oops, ignore the call to Thread.MemoryBarrier().
Why ?
private async Task Background() { while (_done == false) ... }
public void Dispose() { _done = false; ... }
Stephen Toub's advice (http://msdn.microsoft.com/en-us/magazine/hh456402.aspx) is to almost always use ConfigureAwait(false) (ie await task.ConfigureAwait(false)) in library code where you have no need to resume on the same thread. It improves performance, and can help to avert this kind of deadlock. Have you tried that?
I agree with Samuel's advice. Try the code with "await event.WaitAsync().ConfigureAwait(false);".
Unless you have not posted the actual code, I think the background task is 'deadlocking' because the done field is never set to true, to get out of the while loop. So the Dispose call is waiting forever.
// Ryan
ConfigureAwait won't help here. Smaller repro: https://gist.github.com/v2m/5801835. Asyncs infrastructure tries to reust thread that calls SetResult(null) to invoke pending continuation and it results a deadlock
I think you are breaking the async pattern a bit by calling Wait() in Dispose(). I try to have a single Wait() call right at the top of my application and nowhere else.
I've seen similar subtle deadlocks when using Wait() or .Result inside async code. Ideally the language would support Task DisposeAsync() and await on the using statement.
An option is to forget about using and IDisposable, implement DisposeAsync() and await it inside a finally block.
Maybe something like this is a good idea when you need to await a Task in Dispose()
https://gist.github.com/jkells/5802895
Bob, I have, it is nice, but not relevant for what I am doing.
Gopok, You have no idea how many threading issues you can get yourself into in VB6, to start with, the debugger didn't support it. Didn't prevent people from doing that.
Mario, I don't think so, it isn't really that interesting to me at this point.
Duarte, You are correct, but that isn't an issue here, because there aren't actually multiple threads going on.
OmariO, Good point, that is a typo when I wrote the code, it is the other way around, I fixed the issue.
Samuel, You are correct, and it is a good recommendation, but as we are running this from a console application, this doesn't matter.
I think there is also a potential race condition:
10: public void PulseAll() 11: { 12: var taskCompletionSource = tcs; 13: tcs = new TaskCompletionSource<object>();
14: taskCompletionSource.SetResult(null); 15: }
Thomas, That is fine, you'll get called on the next pulse.
Jared Kells is right. Due to quite aggressive in-lining when running the state machine of await/async you should be very careful when calling Wait or Result in "async" methods. The task machine registers a synchronous continuation when waiting will always continue on the same thread as the continuation whenever the default task scheduler is used.
/Thomas O
Comment preview