Cannot resolved scope service ... from root provider

[mrodrigues95]

Describe the bug
I receive the following error when performing a mutation with a validator attached.

Removing .AddFairyBread() fixes the error but that's obviously not the desired result.

Hot Chocolate setup:

services
    .AddGraphQLServer()
    .AddQueryType()
        .AddTypeExtension<UserQueries>()
        .AddTypeExtension<ClassroomQueries>()
    .AddMutationType()
        .AddTypeExtension<ClassroomMutations>()
        .AddTypeExtension<AuthMutations>()
    .AddType<UserType>()
    .AddType<ClassroomType>()
    .EnableRelaySupport()
    .AddDataLoader<UserByIdDataLoader>()
    .AddDataLoader<ClassroomByIdDataLoader>()
    .AddAuthorization()
    .AddFairyBread();

Services:

services.AddPooledDbContextFactory<ApplicationDbContext>(opt => {
    opt.UseNpgsql(Configuration.GetConnectionString("DefaultConnection"));
    opt.UseSnakeCaseNamingConvention();
});
services.AddScoped(p =>
    p.GetRequiredService<IDbContextFactory<ApplicationDbContext>>()
        .CreateDbContext());
services.AddValidatorsFromAssemblyContaining<Startup>();
services.AddHotChocolateServices();
services.AddIdentityServices();

Versions:

• FairyBread: v6.0.0
• HotChocolate: v11.1.0
• FluentValidation: v10.0.4

[rune-vikestad]

This is the exact same issue I found a few days ago, and talked to Ben on Slack about.

As such I've reproduced it in https://github.com/rune-vikestad/fairybread-dependencyinjection-bug

[benmccallum]

This one is really weird. I don't really understand it, but it seems like the working profile, FluentValidator correctly registers the valiator and you can see it on the ServiceProvider in it's Services property while debugging. But in the non-working profile, it's not there.

[benmccallum]

What's also strange is that when FairyBread crawls the services at startup to create the cache of validators in your DI, it sees the instance there, as it doesn't throw like it would if there were none.

[benmccallum]

@JeremySkinner, are you aware of any issues with FluentValidation DI with "ASPNETCORE_ENVIRONMENT": "Development". It's so bizarre there's a difference...

This is very old, but could it be a regression due to a similar thing?
FluentValidation/FluentValidation#564

[JeremySkinner]

TL:DR, somewhere you have a singleton-scoped service that's trying to resolve a request-scoped validator.

@benmccallum so the difference in behaviour is because when the environment is set to Development, ASP.NET will validate scoping issues at runtime to ensure that singleton-scoped objects don't have request-scoped dependencies. When the environment is set to production, the asp.net service provider doesn't perform this check (specficially, it's the ValidateScopes option that's set by asp.net as part of a call to WebHostBuilder.UseDefaultServiceProvider - this is only set when the environment is Development, I'm presuming for performance reasons)

Looking at the stack trace, it appears that somewhere you have something singleton-scoped that is trying to inject a validator (which is request-scoped as of 10.0). This will cause the validator to implicitly become singleton-scoped too, which is why the service provider is flagging this up.

[JeremySkinner]

So I think this is the issue:

fairybread/src/FairyBread/IRequestExecutorBuilderExtensions.cs

Lines 21 to 22 in 76c39cc

	services.TryAddSingleton<IValidatorProvider>(sp =>
	new DefaultValidatorProvider(sp, services, sp.GetRequiredService<IFairyBreadOptions>()));

By registering DefaultValidatorProvider as singleton and passing in sp as the first argument, you're passing the root service provider, rather than the service provider that's for the current http request. When you try and resolve validators inside GetValidators, they end up being instantiated by the root service provider, which is causing the error you're seeing (because validators are registered as scoped, but you're resolving them from a non-scoped service provider).

You'll need to make the DefaultValidatorProvider scoped instead, and then I think this should resolve the problem (although I think you may want to split this up into 2 services, as you wouldn't want the assembly scanning to happen on every request).

Note that even with older versions (where validators were Transient) this could’ve still been a problem if a user injected a request scoped service into a transient validator - you’d have ended up with the same issue. Basically, when it comes to dealing with services in aspnet, singleton scope should be avoided for anything that either could have dependencies injected into it, or calls into the service provider directly. (For example, the example in the readme of injecting a dbcontext would be problematic too, assuming the dbcontext is request-scoped)

Hope that helps clarify what's going on, give me a shout if you need any more info.

Edit: If you want to detect & catch this behaviour inside your integration tests, take a look at what we do when we spin up the web host: https://github.com/FluentValidation/FluentValidation/blob/5c43678a217d5713cde5a0f716e9b46e23c72f21/src/FluentValidation.Tests.AspNetCore/WebAppFixture.cs#L17-L21

[benmccallum]

@JeremySkinner , so glad I tagged you and so very grateful for your help and detailed explanation on this!

Will look to resolve this tomorrow everyone.

[benmccallum]

Hi all, I've just pushed a 6.0.1-preview.1 which fixes this.

There's just one outstanding failing test (which I've currently disabled to get a preview out) which was a test of multiple executions against the IRequestExecutor. On the second and third executions, the IServiceProvider that DefaultValidatorProvider takes as a dependency on is apparently disposed.

Perhaps this isn't a valid scenario. @michaelstaib, should it be possible to call ExecuteAsync on a resolved IRequestExecutor multiple times? Any idea what would be disposing the IServiceProvider?

Hmm, perhaps for my tests to be valid now I need to create my own scope that wraps a test... this stuff's tricky :P
(edit: creating a scope around each execution doesn't work)
(edit2: I probably need to create a new scope around each IRequestExecutor resolution... at that point this test is pretty pointless)
(edit3: yea that works, and it does kinda make sense but will get a second opinion).

[benmccallum]

I've fixed the test by resolving the executor x times for x executions, each time in a new scope. But not really sure if that's valid. If I hear back from Michael I can wrap this up and do a proper release.

When I get time I'll follow @JeremySkinner 's suggestion about integration testing the different DI behaviour in Development as we've also had issues before with serialization that'd benefit from spitting out JSON via HC's ASP.NET middleware, so I'll take the dive.

[michaelstaib]

So if you execution on the IRequestExecutor make sure to dispose the results after you have used them. Apart form that you can use it as long as you want .... Except if you are using schema reloads like in schema stitching. In this case we have a proxy class that implements IRequestExecutor which handles all the reload stuff and so on.

[michaelstaib]

Second, each execution needs a new service scope if you pass in the service provider. If you do not pass in a service provider we will use the main service provider and create a scope internally.

https://github.com/ChilliCream/hotchocolate/blob/79c372debab7ec48a42c616eaa4a58362f18f60b/src/HotChocolate/Core/src/Execution/RequestExecutor.cs#L73

[rune-vikestad]

I'm having issues with 6.0.1-preview.1 after upgrading FairyBread in https://github.com/rune-vikestad/fairybread-dependencyinjection-bug

The createFoo mutation returns as expected the first time i run it, but running it a second time yields the following error;

{
    "errors": [
        {
            "message": "Unexpected Execution Error",
            "locations": [
                {
                    "line": 2,
                    "column": 5
                }
            ],
            "path": [
                "createFoo"
            ],
            "extensions": {
                "message": "Cannot access a disposed object.\r\nObject name: 'IServiceProvider'.",
                "stackTrace": "   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ThrowHelper.ThrowObjectDisposedException()\r\n   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)\r\n   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)\r\n   at FairyBread.DefaultValidatorProvider.GetValidators(IMiddlewareContext context, IInputField argument)+MoveNext()\r\n   at FairyBread.InputValidationMiddleware.InvokeAsync(IMiddlewareContext context)\r\n   at HotChocolate.Utilities.MiddlewareCompiler`1.ExpressionHelper.AwaitTaskHelper(Task task)\r\n   at HotChocolate.Execution.Processing.ResolverTask.ExecuteResolverPipelineAsync(CancellationToken cancellationToken)\r\n   at HotChocolate.Execution.Processing.ResolverTask.TryExecuteAsync(CancellationToken cancellationToken)"
            }
        }
    ]
}

[aradalvand]

@rune-vikestad I'm encountering the exact same problem in 6.0.1-preview.1.
First time, everything works, but running the mutation a second time causes this error:

{
  "errors": [
    {
      "message": "Unexpected Execution Error",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "sendVerificationSms"
      ],
      "extensions": {
        "message": "Cannot access a disposed object.\r\nObject name: 'IServiceProvider'.",
        "stackTrace": "   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ThrowHelper.ThrowObjectDisposedException()\r\n   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)\r\n   at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)\r\n   at FairyBread.DefaultValidatorProvider.GetValidators(IMiddlewareContext context, IInputField argument)+MoveNext()\r\n   at FairyBread.InputValidationMiddleware.InvokeAsync(IMiddlewareContext context)\r\n   at HotChocolate.Utilities.MiddlewareCompiler`1.ExpressionHelper.AwaitTaskHelper(Task task)\r\n   at HotChocolate.Execution.Processing.ResolverTask.ExecuteResolverPipelineAsync(CancellationToken cancellationToken)\r\n   at HotChocolate.Execution.Processing.ResolverTask.TryExecuteAsync(CancellationToken cancellationToken)"
      }
    }
  ]
}

[aradalvand]

@benmccallum Any ideas, Ben?

[benmccallum]

Sorry @rune-vikestad and @AradAral , life's a bit hectic my end at the moment (renovating our place while living in it...).

I'll try look tonight but if you guys have some time to look too I'd appreciate it.

I would've thought that splitting out the crawling of services into IValidatorRegistry which is singleton, from the IValidatorProvider which is now scoped and gets an IServiceProvider injected, would've resolved this. Basically as Jeremy suggested. But I guess for some reason it's passing the same IServiceProvider every time to the IValidatorProvider... so the second time it's used it's disposed...

It's similar to how Jeremy does it in FV, here:
https://github.com/FluentValidation/FluentValidation/blob/main/src/FluentValidation.DependencyInjectionExtensions/ServiceProviderValidatorFactory.cs

[benmccallum]

@AradAral @rune-vikestad , had a brainwave thinking it through. Why am I injecting IServiceProvider into my IValidatorProvider when it's method GetValidators gets IResolverContext as an arg which carries an IServiceProvider. So I made it singleton again and use context.Services now and that seems much better.

preview.2 on the way, sorry again for the delay!

[benmccallum]

Confirmed I can run your repro mutation multiple times too @rune-vikestad and it works. And indeed the unit test now correctly imitates that multi-run scenario, so I'm confident this is fixed.

When I get more time I'll add some asp.net core integration tests to spin up a real gql server and implement Jeremy's suggestion to force the DI Development behaviour to get that all checked as well. Raised as #45.

[benmccallum]

Kinda need to get this out an unlist the original v6 packages so I'm going to assume this is all OK and close it out. Shout out if there's anything I've missed. Cheers!

[rune-vikestad]

I've tested this today both in my own reproducible, and in our development environment - and everything checks out.

Smooth work @benmccallum - tricky nut this one!

[benmccallum]

Awesome. Thanks for the confirmation, and for the reproduction!