Skip to content

System.InvalidOperationException: Collection was modified; enumeration operation may not execute in UseEndpoints. #46758

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

Closed
MistyKuu opened this issue Feb 20, 2023 · 7 comments
Labels
✔️ Resolution: Duplicate Resolved as a duplicate of another issue Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Status: Resolved

Comments

@MistyKuu
Copy link

MistyKuu commented Feb 20, 2023

I have recently upgraded to .net 6 (6.0.13). Application is run under docker containers in kubernetes. For some reason some of the pods randomly fails with System.InvalidOperationException: Collection was modified; enumeration operation may not execute
I'm using multitenancy package https://github.com/saaskit/saaskit which might but not necessarily have to be related to the issue.

In startup this is how it's configured (Configure method):

                app.UseMultitenancy<Tenant>();
                app
                    .UseSwagger(c => { c.RouteTemplate = "docs/{documentName}/swagger.json"; })
                    .UseSwaggerUI(c =>
                    {
                         // swagger setup deleted
                    });

                app.UsePerTenant<Tenant>((tenant, builder) =>
                {
                if (!tenant.Tenant.IsEmpty())
                    {
                        var pathPrefix = string.IsNullOrEmpty(basePath) || !basePath.EndsWith('/') ? "/" : string.Empty;

                        builder.UsePathBase(pathPrefix + tenant.Tenant.Id);
                    }
                    
                   // some other stuff
                
                 builder.UseIdentityServer();
                 builder.UseRouting();
                 builder.UseAuthorization();
                 
                builder.UseEndpoints(endpoints =>
                    {
                        endpoints.MapRazorPages();
                        endpoints.MapControllers();
                        endpoints.MapControllerRoute("signin", "signin", new {controller = "Signin", action = nameof(SigninController.Index)});
                        // some other routes
                    });
                    }

Call stack:

System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.Collections.Generic.List`1.Enumerator.MoveNext()
   at System.Linq.Enumerable.SelectManySingleSelectorIterator`2.ToArray()
   at Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.HandleChange()
   at Microsoft.AspNetCore.Routing.CompositeEndpointDataSource.OnDataSourcesChanged(Object sender, NotifyCollectionChangedEventArgs e)
   at System.Collections.ObjectModel.ObservableCollection`1.OnCollectionChanged(NotifyCollectionChangedEventArgs e)
   at Microsoft.AspNetCore.Builder.EndpointRoutingApplicationBuilderExtensions.UseEndpoints(IApplicationBuilder builder, Action`1 configure)
   at App.IdentityServer.Startup.<>c__DisplayClass8_0.<Configure>b__6(TenantPipelineBuilderContext`1 tenant, IApplicationBuilder builder) in src/App.IdentityServer/Startup.cs:line 601
   at SaasKit.Multitenancy.Internal.TenantPipelineMiddleware`1.BuildTenantPipeline(TenantContext`1 tenantContext)
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
--- End of stack trace from previous location ---
   at System.Lazy`1.CreateValue()
   at SaasKit.Multitenancy.Internal.TenantPipelineMiddleware`1.Invoke(HttpContext context)
   at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext)
   at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
   at SaasKit.Multitenancy.Internal.TenantResolutionMiddleware`1.Invoke(HttpContext context, ITenantResolver`1 tenantResolver)
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.<Invoke>g__Awaited|6_0(ExceptionHandlerMiddleware middleware, HttpContext context, Task task)


It is failing there in UseEndpoints:

var routeOptions = builder.ApplicationServices.GetRequiredService<IOptions<RouteOptions>>();
            foreach (var dataSource in endpointRouteBuilder.DataSources)
            {
                if (!routeOptions.Value.EndpointDataSources.Contains(dataSource))
                {
                    routeOptions.Value.EndpointDataSources.Add(dataSource);
                }
            }

By looking at the code, UseEndpoints is executed after first request is called, not on startup which might cause this issue

@mkArtakMSFT mkArtakMSFT added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Feb 20, 2023
@captainsafia
Copy link
Member

Triage: @MistyKuu Could you provide a complete minimal repro project that uses the problematic package? Ideally something that would reproduce the issue after running dotnet run and launching the app?

@captainsafia captainsafia added investigate Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Feb 21, 2023
@ghost
Copy link

ghost commented Feb 21, 2023

Hi @MistyKuu. We have added the "Needs: Author Feedback" label to this issue, which indicates that we have an open question for you before we can take further action. This issue will be closed automatically in 7 days if we do not hear back from you by then - please feel free to re-open it if you come back to this issue after that time.

@danmoseley
Copy link
Member

might also be interesting to see whether it repros on 7.0 if possible (given #42195 changed relevant code?)

@MistyKuu
Copy link
Author

@danmoseley I believe it won't change anything.
I have solved my issue with wrapping app.UsePerTenant in lock. It's more workaround than solution but works very well.
Background:
The issue here is that combinaton of UsePerTenant and UseEndpoints doesn't work well together. UsePerTenant uses deferred execution and is executed once per tenant, on the very first request to each tenant. It branches app builder, create it's own branch and saves it into concurrent dictionary.
Repro:
Two concurrent requests to two different tenants are executed at the same time.
/tenant1/api/user
/tenant2/api/user2

It implies that two UseEndpoints are called at the same time which throws above exception. Below code isn't thread safe because it shares the same EndpointDataSources collection across multiple requests.

var routeOptions = builder.ApplicationServices.GetRequiredService<IOptions<RouteOptions>>();
            foreach (var dataSource in endpointRouteBuilder.DataSources)
            {
                if (!routeOptions.Value.EndpointDataSources.Contains(dataSource))
                {
                    routeOptions.Value.EndpointDataSources.Add(dataSource);
                }
            }

From my perspective the issue can be closed but I wonder if it might be a completely valid scenario in other use cases and if it's something to look into.

@ghost ghost added Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Feb 22, 2023
@captainsafia
Copy link
Member

@halter73 Any thoughts on this?

@halter73
Copy link
Member

From my perspective the issue can be closed but I wonder if it might be a completely valid scenario in other use cases and if it's something to look into.

I think we should close this as a duplicate of #27465 which is a general issue to better support application multitenancy. Parallel calls to UseEndpoints() using the same DI container (or specifically IOptions<RouteOptions>) is not something that we support.

If each tenant got its own copy of RouteOptions in addition to its own IApplicationBuilder, that might fix things, but I'm not sure.

@halter73 halter73 removed their assignment Mar 14, 2023
@captainsafia captainsafia added ✔️ Resolution: Duplicate Resolved as a duplicate of another issue and removed investigate labels Mar 17, 2023
@ghost ghost added the Status: Resolved label Mar 17, 2023
@ghost
Copy link

ghost commented Mar 18, 2023

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

@ghost ghost closed this as completed Mar 18, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
✔️ Resolution: Duplicate Resolved as a duplicate of another issue Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Status: Resolved
Projects
None yet
Development

No branches or pull requests

5 participants