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 #15

Open
anirudhsanthiar opened this Issue Feb 9, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@anirudhsanthiar

anirudhsanthiar commented Feb 9, 2016

The line
var responseMessage = httpClient.SendAsync(requestMessage, CancellationToken.None).WaitForTask();
in the Upload method of the StandardFileUploader has the potential to cause a deadlock.
In particular, the WaitForTask method blocks on Task objects.
Blocking on Task objects (Via Task.Wait() or Task.Result or Task.GetAwaiter().GetResult())
is dangerous and could result in deadlocks.
(See, for example http://blog.stephencleary.com/2012/07/dont-block-on-async-code.html)

Here is some client code to trigger this deadlock if invoked from a GUI context (such as a button-click handler) or from an MVC context.

  var sfClient = new ShareFileClient("https://secure.sf-api.com/sf/v3/");
  var parentFolder = (ShareFile.Api.Models.Folder) sfClient.Items.Get().Execute();
  var file = File.Open(@"C:\test\image.png", FileMode.OpenOrCreate);
  var uploadRequest = new UploadSpecificationRequest
            {
                FileName = file.Name,
                FileSize = file.Length,
                Details = "Sample details",
                Parent = parentFolder.url,
                Method = ShareFile.Api.Models.UploadMethod.Standard
            };

  var uploader = sfClient.GetFileUploader(uploadRequest, new PlatformFileStream(file, file.Length, file.Name)); 
  var uploadResponse = uploader.Upload();

Other call sites that use WaitForTask are likely to be susceptible too.
Note that the code above won't deadlock a console app, or if called from a unit test. You need to invoke it from the main thread of a GUI app/MVC app.
I shall be happy to supply further details in the case that this report is not clear.

@rgmills

This comment has been minimized.

Contributor

rgmills commented Feb 9, 2016

Hi @anirudhsanthiar thanks for the report. Our synchronous APIs are currently only consumed from console based apps, all other GUI apps end up using our Async APIs, i.e. sfClient.GetAsyncFileUploader.

Are you using this in a .net 4.0 context where you're limited to sync only APIs?

@anirudhsanthiar

This comment has been minimized.

anirudhsanthiar commented Feb 10, 2016

Hi @rgmills...thanks for the acknowledgment. It would be great if you could add a note to the documentation clarifying that GUI apps are only meant to consume the async APIs. Another possible fix is to configure the await statements in ShareFile to not capture context via ConfigureAwait(false) [see, for example, the section under "avoiding context" in 1]

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