Missing ConfigureAwait(false)

[zzzprojects]

In the issue #1111, we discussed on missing ConfigureAwait(false) which has been fixed in the pull #1248

However, I tried again your solution today, but there is still some ConfigureAwait missing like in the file HttpClientAdapter.SendAsync

var clonedRequest = await CloneHttpRequestMessageAsync(request);

should be

var clonedRequest = await CloneHttpRequestMessageAsync(request).ConfigureAwait(false);

If you want, I can create a Pull Request. However, it only takes five mins to fix them all, so I'm not sure if you prefer to verify/fix it yourself.

[ryangribble]

Hi @zzzprojects thanks for the heads up ☝

But the one you highlighted looks ok to me? It's an internal function, and inside that function the only await call DOES have ConfigureAwait(false) on there already?

Happy for you to send a pull request for any you find to discuss/review further

[zzzprojects]

That doesn't matter.

The SendAsync method is asynchronous, and since you don't use ConfigureAwait(false) for this method, the UI will be blocked. So yes, the function inside (CloneHttpRequestMessageAsync) doesn't block the UI however, the line await CloneHttpRequestMessageAsync(request); do.

By example, this method in my WinForm application is blocked if I use your latest version. By just adding the ConfigureAway(false) everywhere it's now working.

public static void CreateGist(string description, Dictionary<string, string> files)
{
    var github = CreateClient();

    // CREATE new gist
    var newGist = new NewGist {Description = description, Public = true};

    // ADD file
    foreach (var file in files)
    {
        newGist.Files.Add(file.Key, file.Value);
    }

    // EXECUTE
    var result = github.Gist.Create(newGist).Result;
}

I may be wrong but a third party library should use ConfigureAwait(false) everywhere the await is used.

Do you want me to put await & ConfigureAwait on the same line whenever is possible? I think there is only one place which I have to keep the multiline for readability.

[shiftkey]

Yeah, I think this is a necessary evil of await for library code - whileCloneHttpRequestMessageAsync has it's own await and ConfigureAwait usage this doesn't mean callers know about it - it only sees Task<T> rather than the narrowed ConfiguredTaskAwaitable<T>.

@zzzprojects I'm concerned that your code using .Result - which will block the current thread - doesn't reflect reality. Are you able to provide a small sample WinForms app which uses async void or equivalent, so I can see this behaviour for myself?

[zzzprojects]

Are you able to provide a small sample WinForms app which uses async void or equivalent, so I can see this behaviour for myself?

The problem is right here. Since the library is missing a few ConfigureAwait and the API doesn't provide any non-asynchronous method, you force people to use an async method for not blocking the UI which is bad.

Nop, I cannot provide any async void that will not work, since if I create an async method, the UI will not be blocked.

Using .Result allows resolving the task for a non-asynchronous application like mine. If I don't call Result, the code will simply continue to run without the task necessary terminated.

Using this example:

// CREATE gist
CreateGist(description, files);

// DO something else after the gist has been created
DoSomethingElse();

Without .Result
The method DoSomethingElse() is called before the gist is created since the task to create the gist is currently running.

With .Result
The method DoSomethingElse is called after the Gist is created because we forced the result/task completion.

The user from the issue #1111 had the same problem as me. I'm pretty sure plenty of people tried your library and stopped because it was "not working". So yes, using .Result when none non-asynchronous method is provided reflect the reality ;)

[shiftkey]

Okay, now I'm confused.

Using .Result allows resolving the task for a non-asynchronous application like mine. If I don't call Result, the code will simply continue to run without the task necessary terminated.

Whether you call .Wait() on the task or use .Result directly, this will block the current thread until the task has completed. This is different to how ICriticalNotifyCompletion (the interface which .ConfigureAwait(false) returns) works around scheduling continuations.

Nop, I cannot provide any async void that will not work, since if I create an async method, the UI will not be blocked.

I'm not sure I follow - if you're awaiting on the result of a task, it will be made available to you when it's ready. Like this:

public static Task<Gist> CreateGist(string description, Dictionary<string, string> files)
{
    var github = CreateClient();

    // CREATE new gist
    var newGist = new NewGist {Description = description, Public = true};

    // ADD file
    foreach (var file in files)
    {
        newGist.Files.Add(file.Key, file.Value);
    }

    // START TASK
    return github.Gist.Create(newGist);
}

// note: this method can also be "async void" but returning
//       a Task is nicer so that callers can await this too
public static async Task DoSomething()
{
    // CREATE gist
    var gist = await CreateGist(description, files);

    // DO something else after the gist has been created
    DoSomethingElse(gist);
}

I'm not clear on the behaviour you're seeing - is it deadlocking, or just blocking the current thread?

[zzzprojects]

It's a deadlocking. The thread is deadlocked with the UI thread.

Look at your code, you FORCED the user to use an async method and that's really bad. Most people doesn't understand how async method work, so that's why I avoid them.

This is what I want:

public static void CreateGist(string description, Dictionary<string, string> files)
{
    var github = CreateClient();

    // CREATE new gist
    var newGist = new NewGist {Description = description, Public = true};

    // ADD file
    foreach (var file in files)
    {
        newGist.Files.Add(file.Key, file.Value);
    }

    // START and COMPLETE TASK
    github.Gist.Create(newGist).Result;
}

public static DoSomething()
{
    // CREATE gist
    CreateGist(description, files);

    // DO something else after the gist has been created
    DoSomethingElse(gist);
}

Imagine a legacy application where the DoSomething method already exists. You forced an existing method to be changed to use now an async Task and to use await operator. Furthermore, if the DoSomething method is now async, every method called it must now be async... and... well, that's just becoming worse and worse.

What could have been few seconds changes by simply calling .Result now become a mess because the library FORCE all methods using the API to also use async method.

Anyway, from my part, I just downloaded the source and added all missing ConfigureAwait(false) which took 5 minutes and I can now use the library with .Result

[shiftkey]

It's a deadlocking. The thread is deadlocked with the UI thread.

This is what I needed to hear. Thanks.

If you're not interested in submitting a PR I'll add this to my task list for the week.

[zzzprojects]

I will be happy to do it tomorrow.

All I need to know is the answer to the following question:

Do you want me to put await & ConfigureAwait on the same line whenever is possible? I think there is only one place which I have to keep the multiline for readability.

[shiftkey]

@zzzprojects single-line is preferred, yes