Skip to content

GetOrCreateAsync's factory should use ConfigureAwait#2767

Merged
Tratcher merged 1 commit intodotnet:masterfrom
ladeak:ladeak-ConfigureAwait
Jan 21, 2020
Merged

GetOrCreateAsync's factory should use ConfigureAwait#2767
Tratcher merged 1 commit intodotnet:masterfrom
ladeak:ladeak-ConfigureAwait

Conversation

@ladeak
Copy link
Copy Markdown

@ladeak ladeak commented Dec 8, 2019

GetOrCreateAsync's factory should use with ConfigureAwait false, as some UI applications might block on the UI thread.

  • Added ConfigureAwait(false) in GetOrCreateAsync
  • Added a test to cover the issue.
  • Added two helper classes for the tests. Helper classes comments' indicated their original source.

Not sure if tests falls into the 'too hard' category, because of the helper classes.

Addresses #2252

@ladeak
Copy link
Copy Markdown
Author

ladeak commented Dec 11, 2019

@anurse and @Tratcher please review this PR.

@Tratcher Tratcher removed their request for review December 12, 2019 02:04
@ladeak
Copy link
Copy Markdown
Author

ladeak commented Dec 16, 2019

@Pilchie please suggest reviewers

Copy link
Copy Markdown

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, but with a few thoughts/questions.

@davidfowl
Copy link
Copy Markdown
Member

@rynowak had some thoughts about ConfigureAwait in these libraries, specifically that its not worth trying to test all of the code paths and instead use an analyzer to make sure all the places have ConfigureAwait(false).

cc @JamesNK

@ladeak
Copy link
Copy Markdown
Author

ladeak commented Dec 16, 2019

@davidfowl , I am happy to with the analyzer instead of tests, could you suggest one?

@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Dec 16, 2019

I have setup Microsoft.CodeAnalysis.FxCopAnalyzers in https://github.com/grpc/grpc-dotnet

It comes with a lot of analyzers. I have disabled most of them but ConfigureAwait false is still enabled: https://github.com/grpc/grpc-dotnet/blob/dd72d6a38ab2984fd224aa8ed53686dc0153b9da/Grpc.DotNet.ruleset#L69

@ladeak ladeak requested a review from a team as a code owner December 17, 2019 12:57
@ladeak
Copy link
Copy Markdown
Author

ladeak commented Dec 17, 2019

I suppose I need to run GenerateReferenceSource or GenerateReferenceAssemblies to resolve build issues.

@ladeak
Copy link
Copy Markdown
Author

ladeak commented Dec 17, 2019

When Directory.Build.target defines the build Reference to FxCopAnalyzer and as well as CodeAnalysisRuleset, I get a build error because ref/*.csproj-s are not updated.

eng/targets/ResolveReferences.targets(200,5): error MSB3245: Could not resolve this reference. Could not locate the package or project for "Microsoft.CodeAnalysis.FxCopAnalyzers". Did you update baselines and dependencies lists? See docs/ReferenceResolution.md for more details.

When I move

  <ItemGroup>
    <Reference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" />
  </ItemGroup>

to Directory.Build.props, and run the script that updates the ref/ projects, then build.cmd, I get the following error:

C:\Users...\source\repos\Extensions.dotnet\sdk\5.0.100-alpha1-015536\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.Sdk.DefaultItems.targets(256,5): error NETSDK1023: A PackageReference for 'Microsoft.CodeAnalysis.FxCopAnalyzers' was included in your project. This package is implicitly referenced by the .NET SDK and you do not typically need to reference it from your project. For more information, see https://aka.ms/sdkimplicitrefs [C:\Users...\source\repos\Extensions\src\Caching\Abstractions\ref\Microsoft.Extensions.Caching.Abstractions.csproj]

What would be the suggested way to resolve this issue?

@ladeak ladeak requested review from analogrelay and removed request for a team December 18, 2019 22:09
@rynowak
Copy link
Copy Markdown
Member

rynowak commented Dec 19, 2019

Looks like we probably have to add this to the references list: https://github.com/aspnet/Extensions/blob/master/docs/ReferenceResolution.md#example-adding-a-new-dependency

@anurse @JamesNK - can you help with that?

@analogrelay
Copy link
Copy Markdown

Step 1 and 2 there should be all that's needed, I believe. Adding the <Reference> element to the project and then the <LatestPackageReference> element to the eng/Dependencies.props file. While this package does comes from "another dotnet team", it's dotnet/roslyn-analyzers which is not part of our dependency flow process, so there's no need to go through step 3.

@ladeak let me know if you have any issues making those updates!

@ladeak
Copy link
Copy Markdown
Author

ladeak commented Dec 20, 2019

@anurse , I have added the Dependency to the "External dependencies":
https://github.com/aspnet/Extensions/pull/2767/files#diff-8c508e8b957801ef7a859521e59d58b9 but still returns a build error:

eng/targets/ResolveReferences.targets(200,5): error MSB3245: Could not resolve this reference. Could not locate the package or project for "Microsoft.CodeAnalysis.FxCopAnalyzers". Did you update baselines and dependencies lists? See docs/ReferenceResolution.md for more details.

@ladeak
Copy link
Copy Markdown
Author

ladeak commented Jan 10, 2020

@anurse Could you suggest, I did those two steps as suggested, but I still get errors.

@ladeak ladeak requested a review from dougbu as a code owner January 17, 2020 18:31
…applications might block on the UI thread.

Added FxCop dependency to Caching projects, and code analysis rules for shipping and non-shipping projects.
@ladeak ladeak requested a review from Tratcher January 17, 2020 19:21
@ladeak
Copy link
Copy Markdown
Author

ladeak commented Jan 17, 2020

@anurse , @Tratcher and @dougbu please complete reviews :)

@Tratcher Tratcher added this to the 5.0.0-preview1 milestone Jan 21, 2020
@Tratcher Tratcher merged commit cb60ad1 into dotnet:master Jan 21, 2020
@Tratcher
Copy link
Copy Markdown
Member

Thanks

maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 11, 2020
maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 11, 2020
maryamariyan pushed a commit to maryamariyan/runtime that referenced this pull request Mar 27, 2020
JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request Nov 17, 2020
@ghost ghost locked as resolved and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants