-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Undo the change to make the IServiceProvider in Configure scoped #4148
Comments
Yes please, this has broken a key part of our system. |
The workaround is trivial. Just FYI, assign app.ApplicationServices instead of injection IServicerProvider into Configure. |
Sorry David, I do not quite follow the workaround. Is there a change required in my startup file, or should I me changing away from injecting IServiceProvider into those background jobs, and use something else? |
Today I assume you are doing this: public void Configure(IServiceProvider provider)
{
new BackgroundJob(provider).Run(); // Something like this?
} There's like various ways to solve it: Using IHostedService https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/hosted-services?view=aspnetcore-2.1 This is the preferred way. Using DI and Configure: public void Configure(BackgroundJob job)
{
job.Run();
} In this model you add BackgroundJob to the container and take services into the ctor. Using ApplicationServices directly in Configure: public void Configure(IApplicationBuilder app)
{
new BackgroundJob(app.ApplicationServices).Run(); // Something like this?
} |
Not quite, The system runs on a super simple CQRS implementation. The specific part that breaks is that CommandResultReactors get run the background, and as I mentioned it just worked. So instead of manually calling the constructor as shown above, I rely in the DI container to do its magic. On this line, you can see where the dispatcher starts running through the reactors. On this line, you can see where the handle methods are actually called. When running in the context of a web application, and requests are present. The exception will be thrown on this line. Because the reactors run behind the scenes, I always in them would request that a IServiceProvider is injected, so that I can create a scope and request things like database contexts from it safely. It is there, that it now breaks with the object disposed exception. I hope this helps explain the problem better, and hopefully why the sample code does not help me out to the point where I can implement a work around. |
Nope, sorry, I don't see how this breaks. What I need to see is what code activates your types. Where does that happen? If my sample code doesn't help you then I don't see how fixing this issue helps you. |
The best way I can explain is that the dependency injection container, activate the types. When a type is required, I simply request it from the di container. Nowhere is the constructor manually called, its all di dependent. |
Sure. This is getting much closer, but it's not all there yet. Where do you resolve the Dispatcher in the web case. I just need to see the composition root not all of the implementation details. |
In a base class, inheriting from controller I resolve it like this.
|
Great (I also found that code)! I have no idea how this specific change affects any of your code. If something is getting disposed it's because of something else. |
Well, the application has been in production since January and has been working without hassles. |
I'm not saying its not another issue or change, but it certainly isn't this one. Are you running things outside of the request scope that were started during a request scope? |
The reactors could possibly still be running after the request has finished, possibly. |
My guess is that you were getting lucky before. The timing of when the per request service provider gets disposed changed in 2.1 but it's still "after the request is over". That has nothing to do with this issue. If you resolve a scoped service using |
David, that explains it better than anything else. I am currently working making changes in code to fix the issue that we currently have in the code base, simply removing the reactors for now. Later on I will need to come up with a better fix. Thanks for the time you put into this, it is greatly appreciated. 👍 |
/cc @muratg @DamianEdwards I'd like to get this patched in 2.1.x |
Moving to 2.1.4 because it's kind of too late for 2.1.3. |
@davidfowl please send the shiproom template for a patch. If it meets the bar, it'll likely be 2.1.5. |
@davidfowl Does My issue relate?
and initializes a context where it is appears similar to IServiceProvider. Its static so I can use it without instantiation throughout the app to quickly and securely log away issues and this will also be used for the audit process as well.
Finally the db,Add(log); is where I get same error as in this issue.
Is this the same issue or would moving logging into the dependency injection ins startup (which appears similar to the above workaround) resolve my issue. |
Where are you calling SetContext? and where are you using the context? Is it outside of a request thread? |
i meet the same exception , but i am not sure it's the same problem. sutartup config
My consumer
Exception:
|
Looks unrelated. This line should be fine:
|
ok, i only got one this exception log in last 3 month, i'll check it for more information. |
@davidfowl if you'd like to bring this to shiproom for a patch release, please fill out the template. Thanks! |
@davidfowl Are you doing this this week? If not, could you please update the milestone? |
Moved |
@davidfowl what is the recommended work-around for this if |
@nwoolls I'd question why you are doing that and not just inject the dependencies that you require. But if you want a work around the HttpContext holds an instance of IServiceProvider that you would be able to use |
@nwoolls This doesn't affect controller injection. You probably always had that bug if it worked in 2.0 and not 2.1. You're likely extending the lifetime of the scoped service provider by mistake. |
@tidusjar I have the same question out to our devs - it is being used as you say, to resolve dependencies instead of using straight up DI. My feel is that the right solution is to use DI directly instead of injecting |
@davidfowl not disagreeing, but this is pretty easy for us to reproduce with 2.1 and we cannot with 2.0. |
What exactly are you doing? |
@davidfowl the current code injects |
What's the lifetime of IDependency? Transient? Scoped? Is IDependency only used in the static method or is stored somewhere and used later (hence the after disposal). |
@davidfowl Transient
It is only used in the static method. It's a static class with helper methods - no state. |
Where are you using it? In the controller method ? |
@davidfowl something like this - I'll work on a sample that reproduces it. Startup.cs
Controller.cs
StaticClass.cs
|
Yea that code is a recipe for disaster. See https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AspNetCoreGuidance.md#do-not-capture-services-injected-into-the-controllers-on-background-threads |
@davidfowl thanks - we'll try those guidelines and see what shakes loose. |
I assume you're intentionally doing a fire and forget here? |
@davidfowl yes that is intentional. |
@davidfowl confirmed - the guidance you linked did the trick for eliminating the "Cannot access a disposed object" error on .NET Core 2.1. Thank you! |
@davidfowl Do you expect to get this in in a week or so? |
This issue has become irrelevant in 3.0 with the move to generic host. I'm closing this issue because we won't be back porting it. Most of the other issues here don't have much to do with this particular change. |
Turns out people were capturing the IServiceProvider here to store in statics. Initially this change was made to make running EF migrations easier but then that pattern was moved into Program.Main. We've had several reports about this breaking things so we should consider reverting it.
Here's the initial change:
aspnet/Hosting#1106
We can consider this for a later patch as well.
The text was updated successfully, but these errors were encountered: