search

AzureADQuickStarts / B2C-WebApp-WebAPI-OpenIDConnect-DotNet

A .NET MVC web application code sample, secured by Azure AD B2C, that calls a Web API
6 stars 5 forks source link

async-await deadlock in OpenIdConnectCachingSecurityTokenProvider.cs #2

Closed paumayr closed 1 year ago

paumayr commented 8 years ago

I keep on getting deadlocks on my dev-machine when using this TokenProvider. The issue is caused by 

OpenIdConnectConfiguration config = _configManager.GetConfigurationAsync().Result;

in RetrieveMetadata. I changed the line to 

OpenIdConnectConfiguration config = Task.Run(_configManager.GetConfigurationAsync).Result;

which does the trick for me, but I'm not 100% sure this is the correct fix. Edit: I should note that I only observed this on iisexpress, but I suspect this to happen depending on the SynchronizationContext in use. Edit2: I can submit a PR if required.

paumayr commented 8 years ago

I guess this commit is related, and will probably fix the Issue I fixed manually above: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/commit/c7f689f294f359493a6de93e3315611a1888fba7

clairernovotny commented 8 years ago

.Result should never be used...ever. The fix above looks like it uses await and appears better.

dstrockis commented 8 years ago

Thanks guys, well fix this.

nboettcher commented 8 years ago

@dstrockis we are experiencing this same deadlock issue. What is the plan for fixing this?

dstrockis commented 8 years ago

Please look for an update to this sample next week that will not use the PolicyConfigurationManager

dstrockis commented 8 years ago

Just kidding, I realized this is not in the PolicyConfigurationManager class but in the OpenIdConnectCachingSecurityTokenProvider.

For now, I've staged some changes in an updated sample with @paumayr's fix, which should help. But it's not the best solution. There are a couple options here that would be better:

I'll be discussing this problem with @brentschmaltz & the team to decide how to proceed. For now, I don't really have a better answer for you guys.

clairernovotny commented 8 years ago

@dstrockis seems like the core issue is that https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet hasn't had a new NuGet package pushed out. It's hard for us to build it because it's strong named and is not the top-most level dependency. The fix appears checked into the master of the repo, but you can see that the latest NuGet release is from 6/23/15: https://www.nuget.org/packages/Microsoft.IdentityModel.Protocol.Extensions

I think all we really need to resolve this is a new build of that library pushed to NuGet.org.

dstrockis commented 8 years ago

@onovotny ah, I didn't realize that, thanks. I was looking for a way to entirely avoid calling the ConfigurationManager synchronously. But looks the the changes in that commit will suffice here.

@polita B2C needs to get this commit onto NuGet. Do you think that's a possibility?

polita commented 8 years ago

@dstrockis @brentschmaltz Have we really not released 4x since last summer? We should probably do a release then. When does B2C need it?

dstrockis commented 8 years ago

Chatted with Polita briefly. Given there's a reasonable workaround for this issue, we'll sync up next week to figure out when we could publish a new release. Stay tuned.

clairernovotny commented 8 years ago

@dstrockis @polita Any updates on getting this published? There are cases where the PolicyConfigurationManager mechanism is still required and multiple OIDC middleware won't work.

Thanks

brentschmaltz commented 8 years ago

@onovotny @dstrockis even with the fix, using .Result could cause a deadlock. If you really need .Result, you should try the following code as by default ConfigurationManager uses WebClient, which exhibits issues with .Result.

HttpClient httpClient = new HttpClient(); var configManager = new ConfigurationManager<OpenIdConnectConfiguration>(authority, httpClient); var config = configManager.GetConfigurationAsync().Result;

clairernovotny commented 8 years ago

The fix in the code does not use .Result. It uses await instead so it's fully correct. It's already in master, just hasn't had any recent NuGet releases in over a year. You can see it in this commit: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/commit/c7f689f294f359493a6de93e3315611a1888fba7

brentschmaltz commented 8 years ago

What I want to communicate is that even with our updated bits, .Result can hang when called from an HttpContext (inside a controller). I suspect it is due to ConfigurationManager using WebClient when only the address is provided. It is best not to use .Result. BUT if you have to for some reason, can't think of one right now. New up an HttpClient and pass it to ConfigurationManager constructor.

clairernovotny commented 8 years ago

@brentschmaltz using .Result is always a bad idea...but where is it being used? Is there some place else that you're referring to, I think that's why I'm confused :)

brentschmaltz commented 8 years ago

@onovotny we are on the same page: .Result is always a bad idea. I am saying same as you. Don't use it unless you absolutely have to.

polita commented 8 years ago

@brentschmaltz We are currently using .Result in our sample here (line 85): https://github.com/AzureADQuickStarts/B2C-WebAPI-DotNet/blob/complete/TaskService/App_Start/OpenIdConnectCachingSecurityTokenProvider.cs

Is there a better way to write this so there's no risk of a deadlock?

clairernovotny commented 8 years ago

@polita not saying it's a clean way, but in that specific scenario where you're using a Task.Run, I believe that .Result is safe. The reason is that using Task.Run should hide the ambient synchronization context from the method being run, so any ConfigureAwait(true) -- the default unless you say false won't deadlock.

polita commented 8 years ago

@chenyan98 Please make the fixes to the B2C-WebAPI-DotNet sample code.

golfguy0082 commented 8 years ago

Just a question....is there any chance of OpenIdConnectCachingSecurityTokenProvider making it into an "official" NuGet package? It feels a little odd having to copy a class from a sample project to get up and running with B2C.

brentschmaltz commented 8 years ago

@onovotny @polita just got back to this and looked a little closer. Since JwtBearer internally hits the wsFedEndpoint which is not available for B2C, it needs an IIssuerSecurityTokenProvider that reaches out to the discovery endpoint. IIsserSecurityTokenProvider is implemented synchronous. In order to work with out the .Result, we would need a complete synchronous solution.

polita commented 8 years ago

@golfguy0082 The OpenIdConnectCachingSecurityTokenProvider is a pretty thin layer on the configuration manager. Feel free to take it and use it.

onionhammer commented 8 years ago

@polita that's what he's asking not to have to do ;)

polita commented 8 years ago

@onionhammer I understand, but after @ChenYan98 's pending changes to that class, it's going to be about 50 lines of code with no logic in it.

golfguy0082 commented 8 years ago

@polita my comment was focused less at the "amount" of code needed and more towards the "out-of-box" experience of using B2C. Having to copy and paste sample code from a github repository (that the user has to discover) creates a significant barrier to entry for new users evaluating the product. I love B2C and I'm hoping to find ways to help encourage others to try it too! Anything we can do to make the product work "out-of-the-box" will help towards that goal.

onionhammer commented 8 years ago

I 100% agree with @golfguy0082, I'm not even sure why we're debating this; it seems obvious

sw-ms-manishparmar commented 7 years ago

Hi all,

Is there any final solution is defined for this issue? as i am also getting same issue in my azure web app.

for now i have tried below line of code in my project that @paumayr has provided at starting of this thread. OpenIdConnectConfiguration config = Task.Run(_configManager.GetConfigurationAsync).Result;

Thanks, Manish

polita commented 7 years ago

@manish-parmar Using .Result should avoid the deadlock for you.

phatcher commented 7 years ago

I've recently started getting the same, so is a definitive fix...

  1. Change the RetrieveMetadata as per @paumayr
  2. Update the Microsoft.IdentityModel.Protocol.Extensions to 1.0.3.42

Can I also echo the idea of making the token cache a package in it's own right.

Also, Azure B2C seems to lack a bit of love and attention or at least urgency e.g. 9 months in beta, no production tenants except us, no progress on client-flow, no progress on third-party API flow, basic components being in sampleware etc, etc.

aloene commented 6 years ago

@phatcher @paumayr. I already use package 1.0.4.403061554 and encountered the same error (lock) and implemented the proposed fix (Task.Run()) but:

EDIT: Just to be a bit clearer, the observed issue is that, at some point in time, we cannot (on an Azure Web API) enter the writelock anymore. So I deduce another call has never returned (never exited the write lock). This leads to a definitive application lock. I have to restart the pool. I can also see a lot of waiting threads on the API in the Azure portal.

jmprieur commented 6 years ago

@brentschmaltz do you have an idea?

mitchellw3 commented 5 years ago

In need

brentschmaltz commented 5 years ago

@mitchellw3 can you elaborate?

derisen commented 1 year ago

Closing as this repo is being archived.