New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential Deadlock #99

Closed
anirudhsanthiar opened this Issue Jan 4, 2016 · 8 comments

Comments

Projects
None yet
2 participants
@anirudhsanthiar

anirudhsanthiar commented Jan 4, 2016

The API for accessing the results of Task synchronously in the SynchronousInvoker should not be public. Blocking on the results of asynchronous methods can cause deadlocks: see, for example,
http://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

A GUI/MVC based client of Tweetinvi that uses this API can deadlock. A short example follows:

   // An example async method
   public async Task CopyFilesAsync(StreamReader Source, StreamWriter Destination)
   {
     char[] buffer = new char[0x1000];
     int numRead;
     while ((numRead = await Source.ReadAsync(buffer, 0, buffer.Length)) != 0)
     {
       await Destination.WriteAsync(buffer, 0, numRead);
     }
   }

   // code that deadlocks
   ISynchronousInvoker syncInvoker = new SynchronousInvoker();
   string UserDirectory = @"C:\testing\";

   using (StreamReader SourceReader = File.OpenText(UserDirectory + "set5draft"))
   {
      using (StreamWriter DestinationWriter = File.CreateText(UserDirectory + "CopiedFile.txt"))
      {
         syncInvoker.ExecuteSynchronously(CopyFilesAsync(SourceReader, DestinationWriter));
      }
   }

Also, all calls to ExecuteSynchronously within tweetinvi, such as synchronousInvoker.ExecuteSynchronously(StartStreamAsync(url)); should either ensure that the awaits within the async argument to ExecuteSynchronously do not capture context, or else ExecuteSynchronously may be refactored to use Task.Run

@linvi

This comment has been minimized.

Owner

linvi commented Jan 6, 2016

The API for accessing the results of Task synchronously in the SynchronousInvoker should not be public. Blocking on the results of asynchronous methods can cause deadlocks: see, for example,

In tweetinvi I have taken the decision to have most of the classes available publicly so that people can do whatever they want with them. Obviously when using a SynchronousInvoker I would expect a user to know how async works.

Also, all calls to ExecuteSynchronously within tweetinvi, such as synchronousInvoker.ExecuteSynchronously(StartStreamAsync(url)); should either ensure that the awaits within the async argument to ExecuteSynchronously do not capture context, or else ExecuteSynchronously may be refactored to use Task.Run

I am not sure what you mean by do not capture context. Could you please provide more details/explanation? Is there anything wrong with ExecuteSynchronously?

Thanks for the feedback!

@anirudhsanthiar

This comment has been minimized.

anirudhsanthiar commented Jan 7, 2016

To explain, first assume you have the following code that defines functions foo and barAsync

foo()
{
   t = barAsync();
   t.Wait();
   ...
}

async Task barAsync()
{
  ...
  Task t = await SomeOperationAsync();
  // some other code
  return t;
}

When foo first calls barAsync, the part up to the first await is executed synchronously. Then, typically, barAsync will create a fresh, incomplete Task object t and return it to foo, and register the code after the await to be called back when SomeOperationAsync actually completes.
After the call back runs and the return statement in SomeOperationAsync executes, t is marked complete.

Now, imagine that barAsync has returned an incomplete task to foo. Should foo choose to perform a blocking wait on this task, then foo's thread is blocked. In the meanwhile, SomeOperationAsync will complete, and look to run its call back (the remainder of barAsync). The default behaviour of .NET is to post the continuation representing the remainder of barAsync back to the original thread's event queue - this is what I meant by "Capturing the context". The only thing that can unblock the blocked thread is this continuation (because it will complete the task t, signalling the Wait()) but the continuation can never run because the thread is blocked on the Wait() - deadlock!

.NET threads record a "context" as part of their local storage in System.Threading.SynchronizationContext.Current. You could think of this as an abstraction for a scheduler. The default behaviour of await statements is to post the corresponding continuation back to this context. To have the continuation run on a threadpool thread instead, we may say await SomeOperationAsync().ConfigureAwait(false), provided the code after the await does not access any object with thread affinity. Since there are many threadpool threads, even if one of them is blocked, the continuation could always run on another (except in the pathological case where every threadpool thread is blocked too).

The reason ExecuteSynchronously is dangerous is that if I'm developing a GUI app, say a Windows Forms app, and I have the code that follows in a button click handler (you can try this), it will result in a deadlock.

  var userStream = Tweetinvi.Stream.CreateUserStream();
  userStream.StartStream();

The scenario is very similar to what I outlined above.
StartStream calls _synchronousInvoker.ExecuteSynchronously(StartStreamAsync()), which in turn waits on the Task object returned by StartStreamAsync

I hope this clarifies things.
Further reading: Please also refer the article I linked in the earlier post, along with this article that has more details/potential fixes. This stackoverflow post has more details about SynchronizationContext

@linvi linvi added the bug label Jan 21, 2016

@linvi linvi added this to the Version 0.9.11.x milestone Jan 21, 2016

@linvi linvi self-assigned this Jan 21, 2016

@linvi

This comment has been minimized.

Owner

linvi commented Jan 21, 2016

Hello there, I would be keen to understand your example but I might miss something here is the program I created to simulate what I understood from your example :

// Console App

using System;
using System.Threading.Tasks;

namespace TestConcurrency
{
    public class Program
    {
        private static bool RUN = true;

        static void Main(string[] args)
        {
            var p = new Program();
            p.Foo();

            Console.WriteLine("Program Done");
            Console.ReadKey();
        }

        public void Foo()
        {
            var t = barAsync();
            t.Wait();
            Console.WriteLine("Done Food");
        }

        private async Task barAsync()
        {
            await Task.Run(() =>
            {
                while (RUN)
                {
                    Task.Delay(2000).Wait();
                    Console.WriteLine("Waiting at " + DateTime.Now.ToLongTimeString());
                }
            });

            Console.WriteLine("Done BarAsync");
        }
    }
}

The program is behaving the way I would expect from it :

Waiting at 2:07:03 PM
Waiting at 2:07:15 PM
Done BarAsync
Done Food
Program Done 

Am I missing something obvious here?

@linvi

This comment has been minimized.

Owner

linvi commented Jan 21, 2016

Reading further in the article I think I do understand what you are referring to. The problem is that in my example I am not using the ThreadPool up to its limit. Would you agree?

@linvi

This comment has been minimized.

Owner

linvi commented Jan 21, 2016

Hi there, so I am still reviewing my code after reading the article.
I would like to have your point of view on this, could we discuss about this on gitter (github chat system). I invited you already in a channel.

Thanks

@anirudhsanthiar

This comment has been minimized.

anirudhsanthiar commented Jan 22, 2016

I've responded over at gitter.

linvi added a commit that referenced this issue Feb 10, 2016

Issue #99
- Changed SynchronousInvoker to execute its tasks outside of the SynchronizationContext
@linvi

This comment has been minimized.

Owner

linvi commented Feb 10, 2016

A very big thank you to @anirudhsanthiar who has been of a great help. I really appreciate the time you spent to help me improve the library.

I can now safely close this issue without any deadlock!

@linvi linvi closed this Feb 10, 2016

@linvi

This comment has been minimized.

Owner

linvi commented Feb 10, 2016

Associated with #124

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment