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
Execution scoped services #648
Comments
I know to ways to workaround this problem: User Context IContextProvider (IHttpContextAccessor) public interface IContextProvider<T>
{
T Get();
}
public class ContextProvider<T> : IContextProvider<T>
{
IHttpContextAccessor contextAccessor;
public ContextProvider(IHttpContextAccessor contextAccessor)
{
this.contextAccessor = contextAccessor;
}
public T Get()
{
if(contextAccessor?.HttpContext?.RequestServices == null) {
return default(T);
}
return contextAccessor.HttpContext.RequestServices.GetService<T>();
}
}
public class FooType : ObjectGraphType<Foo>
{
private readonly IContextProvider<DependencyToResolve> _scopedServiceProvider;
public FooType (IContextProvider<DependencyToResolve> provider)
{
_scopedServiceProvider= provider;
Field<BarType>("bar", resolve: x => _scopedServiceProvider.Get().Magic(x.Source.Bar));
}
} EDIT |
Hijacking UserContext for this seems just wrong, but that's an another topic (#637) In your example _scopedService would still be a singleton as FooType would be part of the Schema and schema would be singleton. You would need to resolve it inside the resolver which you probably meant. |
@pekkah Totally agree with the user context. sure. What I meant to do: public class FooType : ObjectGraphType<Foo>
{
private readonly IContextProvider<DependencyToResolve> _scopedServiceProvider;
public FooType (IContextProvider<DependencyToResolve> provider)
{
_scopedServiceProvider= provider;
Field<BarType>("bar", resolve: x => _scopedServiceProvider.Get().Magic(x.Source.Bar));
}
} What could be nicer way of resolving dependencies in the schema was to have a provider bound to the HttpContext on public class FooType : ObjectGraphType<Foo>
{
public FooType()
{
Field<BarType>("bar", resolve: x => x.Get<DependencyToResolve>().Magic(x.Source.Bar));
// or
Field<BarType>("bar", resolve: x =>(x.Get(typeof(DependencyToResolve)) as DependencyToResolve)?.Magic(x.Source.Bar));
}
} |
Just read #637 public class FooType<T> : ObjectGraphType<Foo>
{
public FooType()
{
// Resolves Dependencies
Field<BarType>("bar", resolve: x => x.Ressources.Request<DependencyToResolve>().Magic(x.Source.Bar));
// or
Field<BarType>("bar", resolve: x =>(x.Ressources.Request(typeof(DependencyToResolve)) as DependencyToResolve)?.Magic(x.Source.Bar));
// Reads data from property bag
Field<StringType>("foo", resolve: x =>(x.Ressources.Get<string>("foo"));
// or
Field<TType>("tFoo", resolve: x =>(x.Ressources.TryGet<T>("foo", default(T)));
// or
Field<StringType>("foo", resolve: x =>(x.Ressources.Get("foo"));
}
} |
It seems like you could use a service locator pattern, though I think that provides a brittle programming model. It would be easy to expose the I've been trying to think of some other metadata based approach but I'm falling short. |
Why the requirement to keep the Schema as a singleton? If it's expensive to create then maybe some stuff can be cached to make it cheap to create. Then a new Schema instance can be resolved per request/execution and lifetimes are respected for all dependencies. |
@johnrutherford I view the expensive part is the "initialization". At present the entire graph is walked and the Schema is built. The Schema creates and internally caches all of its associated GraphTypes so all GraphTypes take on the lifetime of the Schema. It seems like there should be a way to create the primary graph only once but create the resolvers with their dependencies on a per-request basis. |
This really needs doing doesn't it. I'd imagine a lot of people from a C# background will have a strong affinity towards using Entity Framework as their persistence layer/ORM. Here, singleton instances just won't work unless you either: only ever have one instance of your GraphQL server running and no other service modifying the database, or run everything with change tracker disabled (i.e. |
Correct me if I am wrong, but if I inject dbContext into my fooType. Will the db context be used for subsequent requests ( as a singleton)? Even though dbcontext is registered as a scoped service? This would answer my other questions: #729 |
@john1452 yes, at present GraphTypes are created only once for the lifetime of the Schema and resused. So whatever types are injected to GraphTypes also take on the lifetime of that GraphType and subsequently the Schema. |
@joemcbride thanks, I am now passing the IServiceProvider to userContext and resolving all dbcontext through that. Not the best but it does the job :) |
Hi, maybe I'm missing something obvious here, but here is what I've done. Starting with Asp.Net Core 2.0 we have a way of registering a scoped/transient middleware in the application ( see https://docs.microsoft.com/en-us/aspnet/core/fundamentals/middleware/extensibility?view=aspnetcore-2.0 ) This means that you can inject scoped services in the middleware, eg:
In the case of GraphQLSettings you don't have to pass it as a parameter to the middleware, it actually throws exception as explained in the link:
Now here is the part that I'm not sure about. But I feel I'm on the right track (?!) I'm registering the schema, types as singleton(as before), but the queries as scoped:
Here is the AccountsQuery and RootQuery:
This still fails as singleton scoped Schema can't resolve scoped RootQuery/AccountsQuery so I'm doing this in the middleware (and not resolving RootQuery in Schema):
Now ITenantContext is successfully resolved in the AccountsQuery. This still seems weird, does registering the Queries as scoped and resolving them in the middleware makes sense? Edit: this seems to work only for the first request, subsequent requests give this error: |
Can you provide a short snipped/info on how to do this? This is exactly my use case as I need a dependency resolver (Servicelocator) in a generic Thanks! |
@alexandercarls, I've previously done the following with success:
I'm having some DI fun at the moment where I've got the following:
Problem is, the instance of Any suggestions anyone? Can we solve all these issues in one go by baking something into the framework somewhere? I guess as long as the Types are singleton, the resolvers will be creating their own scope and doing ServiceLocator... So I'll always have this problem. I'm not even using EF, I'm just trying to instantiate a HttpClient essentially that also needs access to the current HttpRequest, or a scoped dep I''ve already created from the request that's now stuck in the outer scope. Would another DI framework help me? |
@benmccallum I also enabled a service locator on my I am then using a set of extension methods on ResolveFieldContext to access the resolver. As for you problem, I didn't encoutered that one yet, as I use the "bag" of the HttpContext and then have an "Accessor" service. Thinking about it, instead of creating a new scope, given that we are still in the middleware chain, it should be possible to just "get the existing scope, created by ASP.NET Core". A way certainly would be opening your own scope with a custom lifetime using another DI solution at the beginning of the middleware. But this is not really idomatic. |
@benmccallum, there are two ways I have done this:
Here's an example: public class MyUserContext
{
public IServiceProvider RequestServices { get; }
public IMyService MyService { get; }
public MyUserContext(HttpContext httpContext)
{
RequestServices = httpContext.RequestServices;
MyService = httpContext.RequestServices.GetRequiredService<IMyService>();
}
} Or you could even declare your dependencies on your user context class and then resolve it like this: |
Thanks @johnrutherford, will give it a go. And thanks @alexandercarls too, your abstractions look great! I was also thinking about how we could do this as a framework. Since I'm in the auth space at the moment, we could take the approach that setting up authentication use when you need to provide custom event handlers. I can't remember the type, I'm on my phone, I think it'd be Could we apply that same pattern to GraphQL resolvers? We give the field simply the type of our resolver with a new property |
Here's an idea:
public class FuncFieldResolver<TServiceType, TReturnType> : IFieldResolver<TReturnType>
{
private readonly Func<ResolveFieldContext, TServiceType, TReturnType> _resolver;
public FuncFieldResolver(Func<ResolveFieldContext, TServiceType, TReturnType> resolver)
{
_resolver = resolver;
}
public TReturnType Resolve(ResolveFieldContext context)
{
var service1 = context.ExecutionServices.Resolve<TServiceType>();
return _resolver(context, service1);
}
object IFieldResolver.Resolve(ResolveFieldContext context) => Resolve(context);
} And then set up extension methods so fields could be created like this: Field<FooType, Foo>()
.Name("foo")
.ResolveAsync<IMyService>((context, myService) =>
{
return myService.GetFooAsync();
}); So this would be equivalent to injecting parameters into a controller action in MVC. As an aside, I think we should get rid of |
That sounds really good. I like the parallel with ctor/invoke method injection so it'll make sense to a lot of people and be easy to document. |
@alexandercarls, FYI I ended up with what John posted with the |
Hey @johnrutherford , is this something you're thinking about for v3? |
@benmccallum oh yeah, I forgot about this. I'll create a new issue with my suggestion and tag it for 3.0. Thanks! |
Hey. Accidentally I saw activity in this thread. I was surprised - just a week ago I was just engaged in solving this problem in our project. In principle, all the described solutions were considered. I want to note that in the discussion attention is focused on In general, a fairly general solution was obtained without using a service locator (or execution services which is actually the same). Also it supports arbitrary environments, not only ASP.NET. Now we have singleton schemas with all graphql types also singletons with some scoped dependencies inside. At the same time, our services function not only as ASP.NET server but also has other entry points for request handling (requests from message broker). So onсe configured DI-container with schema and types in it successfully resolves scoped dependencies from different kind of scopes. I will try to do PR in the near future. |
Now we have singleton schemas with all graphql types also singletons with
some scoped dependencies inside
Just to check, unless you are manually resolving and disposing those scoped
services the lifetime of those will be tbat of the parent object!
…On Tue, 21 May 2019, 01:13 Ivan Maximov, ***@***.***> wrote:
Hey. Accidentally I saw activity in this thread. I was surprised - just a
week ago I was just engaged in solving this problem in our project. In
principle, all the described solutions were considered. I want to note that
in the discussion attention is focused on IHttpContextAccessor and ASP.NET,
although this is a special case of environment. I would also recommend to
turn on scope validation
<https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/web-host?view=aspnetcore-2.2#scope-validation>
to check correctness of your code.
In general, a fairly general solution was obtained without using a service
locator (or execution services which is actually the same). Also it
supports arbitrary environments, not only ASP.NET.
Now we have singleton schemas with all graphql types also singletons with
some scoped dependencies inside. At the same time, our services function
not only as ASP.NET server but also has other entry points for request
handling (requests from message broker). So onсe configured DI-container
with schema and types in it successfully resolves scoped dependencies from
different kind of scopes.
I will try to do PR in the near future.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#648?email_source=notifications&email_token=AAB2QMJ3RCWP2LL4WJKG363PWM5DFA5CNFSM4E54FCQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV2MUXQ#issuecomment-494193246>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAB2QMIEJUOJQQ6SYSMH4D3PWM5DFANCNFSM4E54FCQQ>
.
|
Sample of including the service provider inside the ResolveFieldContext in pull request #1345 |
Adding my two cents here: Unfortunately Identity makes heavy used of Scoped DbContext and to switch it to Transient would require rewriting some fundamental Identity library code. We are experiencing a myriad of issues where DbContext seems to behave as a Singleton when set to Scoped. We believe this is because of the DocumentExecuter being a Singleton. Our application is becoming deadlocked because DbContext is being reused on every request. Obviously this is a huge problem for us. Until we can find a workaround for this or the GraphQL document executor can be used as Scoped, we are probably going to stop using GraphQL and go back to RESTful endpoints. |
What is a problem with |
@joshuapetryk You've got a couple easy solutions here:
public static class ContextExtensions
{
public static T GetService<T>(this ResolveFieldContext context) {
return ((Schema)context.Schema).Services.GetRequiredService<IHttpContextAccessor>().HttpContext.RequestServices.GetRequiredService<T>();
}
}
//resolve: (context) => {
// var text = context.GetArgument<string>("text");
// context.GetService<MyGraphService>().SendEmail(text);
//} See #1345 (comment) Another variation of method 2 is to inject "service resolver functions" via DI into the constructor of the graph, and then within the graph field resolvers, call the "service resolver function" to return a reference to the scoped service. This has the benefit of showing all dependencies in the constructor of the graph, as would be typical for DI-based objects. Just imagine lazy-loaded DI services and you get the picture. @sungam3r has a project that can assist with that type of setup here: https://github.com/sungam3r/SteroidsDI There's a few other ways to attack this problem, each with their pros and cons, but the couple listed above should help you get going without having to change any of your other services' DI configuration. |
We call the DocumentExecuter as part of a GraphQlController. EF Core DbContext when set to Scoped lifetime is only newing on first web request and all subsequent web requests are utilizing the same DbContext instance. When switching DbContext to transient it behaves correctly and news up with each dependent service. This seems to suggest that DbContext when Scoped is actually following the Singleton behavior of DocumentExecuter and not being released after web requests finish. Thanks for the suggestions @Shane32! We have implemented the following workaround and are testing now: // Fix for Scoped lifetime
public class ScopedObjectGraphType : ObjectGraphType
{
IHttpContextAccessor _httpContextAccessor;
public ScopedObjectGraphType(IHttpContextAccessor httpContextAccessor)
{
_httpContextAccessor = httpContextAccessor;
}
protected T GetService<T>()
{
return _httpContextAccessor.HttpContext.RequestServices.GetService<T>();
}
}
// Graph class
public class CustomQuery : ScopedObjectGraphType
{
public CustomQuery(IHttpContextAccessor httpContextAccessor) : base(httpContextAccessor)
{
FieldAsync<CustomType>(
name: "custom type",
description: "description here",
arguments: new QueryArguments(
new QueryArgument<NonNullGraphType<IntGraphType>> { Name = "id" }),
resolve: async context =>
{
var id = context.GetArgument<int>("id");
var dbContext = base.GetService<AppDbContext>(); // Scoped lifetime honored here
// do work
return await stuff;
});
}
} Previously we were injecting |
@joshuapetryk This is not built-in EF Core behavior. Probably you misconfigure DbContext such a way that it become to act as singleton. Please show your configuration before changes. |
I'm guessing he was just trying to use constructor injection .NET Core 3+ does a better job of failing/alerting on these kinds of things upfront rather than us having to discover it at runtime when some weirdness is happening. So I'd be interested what he's running on. Glad you found a solution anyway. I'm still abusing the IServiceProvider I've got on my UserContext (which is scoped to the http request) and have extension methods to make it a little cleaner from |
I think too. |
If |
We are .Net Core 2.2 - thanks for the help! This was a very obscure behavior for us. Edit: looks like this fixed our issue! |
Some months ago I created a .net core graphql project. In the real-life, you need to tune the original library. I want to share some tricks and common classes that help you to build a .net core graphql solution. For example, I've created an automapper that automatically convert your dto model into a ObjectGraphType (output) and InputObjectGraphType (input). But not only this. For example you can't use efficiently dbcontext if you register it using AddDbContext (scoped) like a classical .net core web api, because you need to register Graphql middleware as singleton, so AddDbContext is a "fake scoped" because it is used inside a "singleton middleware". This means that context using AddDbContext is instantiated just once at startup! So, you will have problem of simultaneous access to the connection. But I have solution: use factory pattern. If you want a complete code example, see here: #576 (comment) |
Here a full implementation on .net core 3.1 |
Implemented @johnrutherford 's suggestion in #1151 via #1730 |
In fact not all suggestions. Resolvers injection is a good idea too. |
It seems like a lot of work has been done to work around some strange design decisions... To me, the fundamental problem here is that the
A
If this design decision had not been made, the Schema would remain free to exist as a Singleton. But since the Schema is also responsible for resolution, the conflation mangles together system-level concerns (i.e., types) with runtime implementations (i.e., database queries). In other words, the fact that Schema "wants to be" a singleton while Databases "want to be" transient is clear indication of code smell. This introduces an illogical coupling which is at the root of all the problems described here. |
You can write code schema-first, which behaves in a pattern more like you describe. I prefer to write my code in a MVC-style, where the schema is inferred from the code. However, it seems to be much too complex at present. I'm working on a separate project which will allow a more traditional "MVC-style" approach to writing graphs. See PR #1376 |
I'm familiar with schema-first, but that does not mean that the schema implements business logic. In fact, it remains an anti-pattern, even in schema-first code -- as you can see by looking at Apollo's docs, which is the canonical schema first implementation. You won't find any database queries in your Apollo schema declarations.
Agreed, I prefer this as well. However, it necessitates two things:
... I'm a little hesitant to mention, so please don't think ill of me, but I built Airbnb's GraphQL implementation in ~2015 (?). I worked directly in collaboration with the Facebook folks, back when GraphQL was pretty new. We spent a fair bit of time discussing this exact topic ;) i.e., the relationship between schema and code, so I'm having some serious "Whiteboard deja-vu." |
No problem; I'm quite interested in your viewpoint. Can you explain a little further here:
I'm not sure I quite follow you. |
Happy to. At Airbnb, our concern was fast moving developers who did not take the end-client into consideration. I started as a mobile app developer, and my backend friends tended to forget that we mobile developers couldn't just re-deploy the client app if a field type changed. So as I moved into backend and worked on GraphQL, we discussed how to ensure that our deployments were "regression-proof." To that end, we concluded that the GraphQL schema MUST stand on its own, in a static file, that can be analyzed by any well formed GraphQL client. If you want to derive your schema from code, this means that your output should be a static file that's deployed to the server, and not runtime generated. This adheres to the same principles as a package manager which employs version locking, or a Dockerfile -- which is where the term "hermetic deployments" come from. Hermetic deployments imply a guarantee that, no matter what, your build will always act in the exact same manner (i.e., no external dependencies). The problem with runtime generating a schema file is that there is simply no way to be sure that you will get the same results every time. Even if you're using reflection of class types, it does not guarantee that your dependencies or downstreams have not changed, so you will always run the risk of generating a different schema than you intended and thereby breaking the clients. ... which brings us to the second point. Assuming that your schema file is generated as (for example) a built artifact by your CI tooling, then now you can use any well-formed GraphQL client to tool against regressions. |
In this project, CI tests generate a definition of every public/protected member exposed by this API. Then it checks that against a known definition to be sure that a PR does not change the exposed API. Is that comparable to what you are describing? So during the CI tests, a schema file would be generated and compared to an approved schema file, so that if any changes are made (be it directly or indirectly), it is known and approved. This of course assumes that once the code is built and published, the runtime-generated schema will not change. (e.g. nobody changes the dll files after the project is built and published) |
That sounds like a good solution, yeah! The key bit being: the CI generates an artifact, a.k.a., an actual file or something which is "static" and cannot be changed, tied to that exact version of the app. Sounds like you've got that covered:
Ah, but here concerns me:
So does the code generate the schema at runtime? Or pull from a file generated by the CI?
The project's DLLs are not the only thing that would change a runtime-generated schema. Consider, for example, what happens if my project uses models imported from another DLL. I could upgrade that DLL without upgrading my project, thereby changing the schema. In my scenario, with a static schema, the server might experience errors coercing the old type into the new type. But that's a good thing! Server errors are much easier to catch than client errors, and can be rolled back quickly. In a runtime-generated scenario, the mobile apps just crash mysteriously. edit granted, as I think about this, it seems less likely with a standard C# project since the |
... as I reflect more, I recall that part of our problems had to do with the fact that we were on Ruby on Rails, which is much more susceptible to this kind of problem. You can monkey patch classes, redefine types, etc. at runtime with Ruby (like most scripted languages). This can dramatically alter the generated schema. So if you're confident that the schema is derived from 100% static analysis (e.g., reflection) rather than "true" runtime (e.g., calling user functions), I would agree that your point about the DLL holds. |
This is exactly what we do on dozens of our backend services. It works well. It looks like this [Test]
public void Service_Schema_Should_Be_Approved()
{
using var scope = _serviceProvider.CreateScope();
var serviceSchema = scope.ServiceProvider.GetService<SomeBackendServiceSchema>();
var schemaPrinter = new SchemaPrinter(serviceSchema, new SchemaPrinterOptions
{
IncludeDeprecationReasons = true,
IncludeDescriptions = true,
});
string schema = schemaPrinter.Print();
schema.ShouldMatchApproved(opts => opts.WithFilenameGenerator((_, p1, type, p2) => $"backend.{type}.schema"));
} See Shouldly for |
Out of curiosity, does your introspection query end up serving the |
Introspection query to "live" service is handled as any other graphql request. No static file or |
This issue keeps coming up. Would be good to have a way of resolving dependencies per execution while keeping the schema as singleton. This would require a way of injecting the services in a away that their lifetime would be one execution. For example creating entity framework context per execution.
The text was updated successfully, but these errors were encountered: