Remove Wait call to prevent deadlock

[khellang]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open
• I have verified that I am following the Nancy code style guidelines
• I have provided test coverage for my change (where applicable)

Description

The call to Task.Wait() causes a deadlock on many concurrent requests. This changes the method to do a synchronous copy instead.

Fixes #2533

[grumpydev]

This would imply we have a task that's not CAF'd somewhere?

[jchannon]

CAF'd?

[grumpydev]

I was trying to not have to type ConfigureAwait(false) ;)

[khellang]

This would imply we have a task that's not CAF'd somewhere?

Yes, probably. How do you track those down? It would be interesting if we found it and it fixed the problem.

[khellang]

I think it might be time to look at #1920 again...

[grumpydev]

There's this:

https://github.com/aelij/ConfigureAwaitChecker

But I'm not sure that can be run as a solution wide (or project wide) analysis.

[xt0rted]

A few months back I ran the ConfigureAwaitChecker on the project. The only thing that stood out to me was #2471

[jchannon]

We merging this bad boy? @NancyFx/most-valued-minions

[thecodejunkie]

@jchannon if we think that there is a stray ConfigureAwait(false) somewhere and that's the real cause, then I hope not

[xt0rted]

This might be worth setting up https://github.com/DotNetAnalyzers/AsyncUsageAnalyzers

One of the analyzers in there is for ConfigureAwait()

[jchannon]

Go go go :)

On Friday, 12 August 2016, Brian Surowiec [email protected] wrote:

This might be worth setting up https://github.com/DotNetAnalyzers/
AsyncUsageAnalyzers

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2534 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGapi4Ct_ZhDdwwKhiW0PxI1YEF_X23ks5qfKiKgaJpZM4JiByT
.

[jchannon]

So I added this to Nancy's project.json and ran dotnet build but I get no warnings about async stuff. Is this all I have to do to enable the analyzers?

"AsyncUsageAnalyzers":{
     "version":"1.0.0-alpha003",
      "type":"build"
}

[thecodejunkie]

@jchannon wouldn't that just let the compiler know it's a buildtime dependency? Wouldn't you have to tell something (Roslyn?) to use it to analyze the code ?

[jchannon]

No idea I can't find any info on how to use these analysers on Google. I
found something on style cop but guess that's different. That says you have
to point to a rule set location but not sure these async ones have these

On Saturday, 13 August 2016, Andreas Håkansson [email protected]
wrote:

@jchannon https://github.com/jchannon wouldn't that just let the
compiler know it's a buildtime dependency? Wouldn't you have to tell
something (Roslyn?) to use it to analyze the code ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2534 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGapuCvQaTmKkGV-qeMD6VmA2LVe8Ixks5qfaC-gaJpZM4JiByT
.

[xt0rted]

It looks like there's no tooling to setup/configure code analysis when you use Nancy.Next.sln (unless it's been moved) but it's there when using Nancy.sln. I'm putting together a PR for this right now that sets some of it up.

[jchannon]

So are we confident after @xt0rted PRs etc that there is no CAF issue?

[jchannon]

👀

[thecodejunkie]

@khellang could we update this please.. unsure if we should be merging or closing it? Thanks ❤️

[khellang]

Update? I think this should be merged ASAP.

[thecodejunkie]

Done

[davidallyoung]

@thecodejunkie @khellang I believe this pull request was overwritten during the merge of #2502 specifically by commit 6e46bc4 . Should I make a PR for the change that was intended by this pull request to prevent the deadlock? I discovered this while encountering a similar issue described by the OP in #2533 and wasn't sure why I was still experiencing it, but it looks like this PR was overwritten.

[khellang]

Hmm. Yeah, that's really bad. No clue how that got past a code review 😕

[thecodejunkie]

😢 @davidallyoung mind re-sending the fix?

[davidallyoung]

Sure, I should be able to get it in this evening. @thecodejunkie