Ayende @ Rahien

Refunds available at head office

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 AsyncEvent
   2: {
   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 : IDisposable
   2: {
   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:

image

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.

Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

Bob
06/17/2013 10:02 AM by
Bob

Hmm, have a look at TPL Dataflow - https://nuget.org/packages/Microsoft.Tpl.Dataflow. It can save you all this hard code.

Gopok Phuong
06/17/2013 10:39 AM by
Gopok Phuong

I think you should use better soft language. I have use MS VB6 with lot of happy without this problems.

MARIO
06/17/2013 10:39 AM by
MARIO

Ayende, we can have a reviewing of TPL Dataflow at https://nuget.org/packages/Microsoft.Tpl.Dataflow?

thanks

Duarte Nunes
06/17/2013 11:14 AM by
Duarte Nunes

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 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); }

Duarte Nunes
06/17/2013 11:15 AM by
Duarte Nunes

Oops, ignore the call to Thread.MemoryBarrier().

OmariO
06/17/2013 11:42 AM by
OmariO

Why ?

private async Task Background() { while (_done == false) ... }

public void Dispose() { _done = false; ... }

Samuel Jack
06/17/2013 12:59 PM by
Samuel Jack

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?

Dmitry
06/17/2013 08:39 PM by
Dmitry

I agree with Samuel's advice. Try the code with "await event.WaitAsync().ConfigureAwait(false);".

Ryan Heath
06/17/2013 10:01 PM by
Ryan Heath

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

Vladimir
06/18/2013 12:57 AM by
Vladimir

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

Jared Kells
06/18/2013 05:21 AM by
Jared Kells

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.

Jared Kells
06/18/2013 05:42 AM by
Jared Kells

Maybe something like this is a good idea when you need to await a Task in Dispose()

https://gist.github.com/jkells/5802895

Ayende Rahien
06/18/2013 08:11 AM by
Ayende Rahien

Bob, I have, it is nice, but not relevant for what I am doing.

Ayende Rahien
06/18/2013 08:12 AM by
Ayende Rahien

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.

Ayende Rahien
06/18/2013 08:13 AM by
Ayende Rahien

Mario, I don't think so, it isn't really that interesting to me at this point.

Ayende Rahien
06/18/2013 08:13 AM by
Ayende Rahien

Duarte, You are correct, but that isn't an issue here, because there aren't actually multiple threads going on.

Ayende Rahien
06/18/2013 08:14 AM by
Ayende Rahien

OmariO, Good point, that is a typo when I wrote the code, it is the other way around, I fixed the issue.

Ayende Rahien
06/18/2013 08:17 AM by
Ayende Rahien

Samuel, You are correct, and it is a good recommendation, but as we are running this from a console application, this doesn't matter.

Thomas Olsson
06/23/2013 06:02 PM by
Thomas Olsson

I think there is also a potential race condition:

10: public void PulseAll() 11: { 12: var taskCompletionSource = tcs; 13: tcs = new TaskCompletionSource();

           Assume that the Background task is done with its work here and calls WaitAsync now. It will then get the new tcs, which is not the one that is triggered by this pulse.

14: taskCompletionSource.SetResult(null); 15: }

Ayende Rahien
06/23/2013 10:20 PM by
Ayende Rahien

Thomas, That is fine, you'll get called on the next pulse.

Thomas Olsson
06/24/2013 07:08 AM by
Thomas Olsson

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

Comments have been closed on this topic.