-
Notifications
You must be signed in to change notification settings - Fork 209
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
UpdateMergedOptionsFromMicrosoftIdentityOptions Collection was modified; enumeration operation may not execute. #1957
Comments
Thanks @RbsTechOnline |
@jmprieur it sure is, ill try and report back. Thanks |
Thanks @RbsTechOnline |
Currently running into the same issue, tried the 2.0.5-preview package but the same exception still occurs. |
@bartdebever Does the error happen in .NET 6 as well? |
@RbsTechOnline and @bartdebever I'm not able to repro on .NET 6 or .NET 7. can you provide exact repro steps, I tried to mimic what @RbsTechOnline did by replicating what he has provided above, but could be missing a key part. I'm running our web app calls web API calls graph sample. |
This used to work within .NET 6, same package version. I'll attempt to reproduce the issue myself within a new project on GitHub as I, unfortunately, can't share the source for the actual project. |
Hi @jennyf19 I can verify the same error in I do note the stack trace is slightly different,
I cant seem to find the 2.0.5-preview tag to download the source. However, as soon as i get a chance, I'll try to get repro if one isn't already tendered |
We have the same issue |
Hi guys, I am having the same issue here. Did anyone find something around it? |
@bartdebever, @bartdebever @Eneuman @RbsTechOnline @evandromendonca Do you configure claims actions? |
Hi guys, hi @jmprieur. Unfortunately, I don't have repo with repro steps. But I could narrow down the issue to Swagger "Swashbuckle.AspNetCore". The issue was that the So this did the trick for me: app.UseAuthentication();
app.UseAuthorization();
// and then:
app.UseSwagger();
app.UseSwaggerUI(); I didn't have the time to drill down exactly what was causing the issue due to tight deadlines here, but if it is still a thing I can try in the future. I hope this can help you guys. Cheers! |
I was experiencing the same problem consistently after upgrading from .net 6 to .net 7. I have a React client with a .net 7 api. When the application loaded the first time by entering in the URL into the browser and hitting the enter key it all worked as expected. If I then click the browser refresh button, I was getting the same error above. As long as I hit the refresh button, I would get the same error. If I typed the URL into the address bar and hit enter after it errored, it would work as expected. Not sure why that would trigger the error. Also, I thought it was swagger also initially, but I commented that all out and was still receiving the error. I then upgraded to Microsoft.Identity.Web 2.0.5-preview and I thought the problem was resolved but it was not. After upgrading I can still reproduce the error if I start the api, and right after visual studio appears to be running the api, but the api is not actually ready according to the console (i.e. run api as Output Type = Console Application) and then refresh the browser before the api is ready in the console, then I get the error. If I then type in the url manually it works and then refreshing works after. It appears to be an issue only if api is not really ready to take requests even though visual studio appears to have started up. Viewing the console in this case is helpful. I have not downgraded to Microsoft.Identity.Web 1.25.5 to see if it behaves the same with the previous version. UPDATE: I downgraded back to 1.25.5, and reverted everything back to what it was just after upgrading to .net 7.0 and the issue only happens when I attempt to reload the application when the api is not ready as described above. If I wait for everything to load in the console including listing all of the entities for EF warnings, then it works as expected with Microsoft.Identity.Web 1.25.5 . |
Thanks for all the suggestions and repro steps. Does anyone want to see if this branch resolves the issue? I'm still working on a repro of the issue. |
@jennyf19 I have notable problems with this in development of a Serverside Blazor and and mostly when app is restarted during hot reload. Using the branch https://github.com/AzureAD/microsoft-identity-web/tree/jennyf/fix1957 I still have the issue unfortunately. I am not well versed in this type of code but I noted that the As illustrated by this screenshot with duplicate DeleteClaimActions with the same ClaimType and ValueType. |
I think contains will check if the exact instance exists in the collection so if any claims mapping has been done, this will probably fail. A more robust implementation would be to check if the claim type and value exists. |
@jennyf19 the good news is that the fix no longer produces an error merging the ClaimsActions here: microsoft-identity-web/src/Microsoft.Identity.Web.TokenAcquisition/MergedOptions.cs Line 74 in ecab0d8
The bad news is that we just moved the same type of error to line 214 and the merging of Scopes microsoft-identity-web/src/Microsoft.Identity.Web.TokenAcquisition/MergedOptions.cs Lines 210 to 216 in ecab0d8
|
I think I am able to force the error to occur in a brand new Blazor application. I don't know if this is considered "clear repro steps" but...
|
@magols Those steps helped a lot. I have updated the branch. |
This fix is not on a branch according to git. Basically anytime any of my APIs shut down for inactivity and then restart, the error ALWAYS occurs. It happens always on initial load of the api. It goes away after that but as soon as it shuts down (for inactivity) and restarts, it happens again. I know you are trying to fix this but does Microsoft not place any priority on this library? Is this not a fundamental piece of their software? This is the second major release (6,7) where I have had to deal with the Microsoft Identity libraries. Right now this issue should have had as many people rushing to fix it as fast as possible. If I did this to my clients they would all fire me. Right now I am going to spend the next couple of hours downgrading to .net 6 to resolve this for them and it would be nice if Microsoft would put the same effort for libraries that are fundamental to their clients. When 6 came out, Maui with IOS would not work with Microsoft.Identity.Client for months. My only option in this case is to downgrade back to 6 until this issue is resolved. I find it hard to believe that no one has seen this issue before releasing .net 7, as it occurs in 4 of my api projects and is easily reproducible. I can send you my production logs to show you if you like but it is essentially the same error as reported above. |
@billyjacobs2014 what do you mean not on a branch? We worked together on it yesterday night with Jenny. We have been prioritizing fixing it, but had not been able to repro it until @magols's provided clear repro steps The fix is in jennyf/fix1957. |
@jennyf19 |
I was having the same issue on my API for a Blazor WASM project after updating to .net 7. |
We believe we have a fix, which we'll release tomorrow PST. |
+1 to @f1nzer |
1.25.7 is released. We will release the fix in v2-preview as well. |
@jennyf19 @jmprieur
|
which same test @f1nzer? you mean the unit test? or the test with dotnet watch? |
@jennyf19 @jmprieur Just FYI, in my API, it used to happen (beginning after upgrade to .NET 7) every time it started up (when hit first from web app - multiple connections at once, no issues when hit first from single swagger connection, as documented above)
|
It can be reproduced with the same unit test I sent you before. |
@f1nzer we have a fix in master now, I'm not able to repro it with the test you provided, even with 100k concurrent calls. |
I want to confirm that as of the latest release (1.25.9), this issue has indeed been fixed and working in a production scenario. |
Thanks for confirming @bartdebever |
The linked merged request #1979 is still open and not merged to master yet? And the source code download for the 1.25.9 release doesn't show it either. Has this actually not been resolved in the latest nuget release or have I misunderstood something? |
I updated to version
Should I open a new issue for this? I can maybe try downgrading to 1.25.9 but I'm quite confused. Is the fix only available in .NET7? |
the .NET 7 fix should be released mid February. |
Thanks @jmprieur, I'll implement the suggested workaround for now. Will the fix also make it to the LTS .NET6 version as well? I thought it was a package problem not in the runtime but just curious if I need to wait for .NET7 or 8 to remove the workaround. |
@rondelward-pf : it's a regression in .NET 7. |
@jmprieur You wrote, that's a regression in .NET 7, but you did not specifically anwser the question of @rondelward-pf, if this fixes it for .NET 6, too . What confuses me too, is that the Fix was made in "Microsoft.Identity.Web", so why did you point out the next .NET 7 runtime patch? Shouldn't the commit fix it by itself?
This is one of these log entries, using V2.13.4
|
Microsoft.Identity.Web Library
Microsoft.Identity.Web
Microsoft.Identity.Web version
1.25.5
Web app
Sign-in users and call web APIs
Web API
Protected web APIs (validating tokens)
Token cache serialization
Not Applicable
Description
Fist call to web api is causing an error an
Collection was modified; enumeration operation may not execute
in
internal static void UpdateMergedOptionsFromMicrosoftIdentityOptions(MicrosoftIdentityOptions microsoftIdentityOptions, MergedOptions mergedOptions)
Seemingly at
if (!mergedOptions.ClaimActions.Contains(claimAction))
Reproduction steps
This is just a preliminary assessment. Can work towards a full repro if needed
Error message
Id Web logs
No response
Relevant code snippets
Regression
No response
Expected behavior
I am not really sure where the problem is originating from, or what might be the cause of it.
My questions is, is this known issue, and I am missing something simple?
The text was updated successfully, but these errors were encountered: