Skip to content
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 #2048

Closed
anirudhsanthiar opened this issue Mar 22, 2017 · 3 comments
Closed

Potential Deadlock #2048

anirudhsanthiar opened this issue Mar 22, 2017 · 3 comments
Assignees

Comments

@anirudhsanthiar
Copy link

@anirudhsanthiar anirudhsanthiar commented Mar 22, 2017

In https://github.com/Azure/autorest/blob/master/src/core/AutoRest.Core/Template.cs, we have

public override string ToString()
        {
            var sb = new StringBuilder();
            if (Model != null)
            {
                var existingOutput = TextWriter;
                using (TextWriter = new StringWriter(sb, CultureInfo.InvariantCulture))
                {
                    ExecuteAsync().ConfigureAwait(false).GetAwaiter().GetResult();
                }
                TextWriter = existingOutput;
            }

            return sb.ToString();
        }

This method could deadlock because of the blocking call to GetAwaiter().GetResult(). Note that the call to ConfigureAwait(false) here has no effect, because it is meant to configure await statements (and not tasks).
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)

The following code that calls the ToString method above will deadlock from a windows forms/ASP.NET MVC app - to reproduce, set up a windows forms class, say Form1, with a button click handler that calls DeadlockToString

class SampleModel : AutoRest.Core.Template<Form1>
    {
        public async override Task ExecuteAsync()
        {
            await Task.Delay(100);
        }

        public override String ToString()
        {
            return base.ToString();
        }
    }

 public partial class Form1 : Form
{
    public static void DeadlockToString()
    {
            var sampleModelTemplate = new SampleModel();
            sampleModelTemplate.Model = this;
            var output = sampleModelTemplate.ToString();
    }
    
    private void button1_MouseClick(object sender, MouseEventArgs e)
    {
        DeadlockToString();
    }
}    

A possible solution is to run ExecuteAsync by passing it as a delegate to Task::Run in ToString. Please let me know if you need additional information.

@olydis
Copy link
Contributor

@olydis olydis commented Mar 23, 2017

We're aware of this spot and you're absolutely right about the semantics of it. Note, however, that this is the AutoRest core you're talking about - we control the app consuming it (a command line tool that will even be replaced soon). We didn't design the core as a standalone library that we expect others to use - it is in constant motion. Are you actually planning to use the core in the scenario you describe, or is this issue purely hypothetical?

@anirudhsanthiar
Copy link
Author

@anirudhsanthiar anirudhsanthiar commented Mar 24, 2017

I was using the library from a GUI context, when I ran into the issue. The repro above is a simplified scenario showing the bug.

@olydis olydis self-assigned this May 3, 2017
@olydis
Copy link
Contributor

@olydis olydis commented May 3, 2017

We indeed don't provide support for using the library as a standalone - in this case I'd suggest wrapping the call to ToString with a Task - basically what you suggested, just at the call site. For us, "fixing" this within ToString doesn't really make sense since we don't hit the deadlock and try to be as lightweight as possible (note that the Task solution also makes assumptions about thread pool configuration and such).

@olydis olydis closed this May 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.