-
Notifications
You must be signed in to change notification settings - Fork 469
Fixing /admin/host/resume ObjectDisposedException #11264
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
Conversation
test/WebJobs.Script.Tests.Integration/WebHostEndToEnd/DrainModeResumeEndToEndTests.cs
Show resolved
Hide resolved
Added a bunch more context to the description; leaving this in Draft now until I can discuss with @fabiocav. It's possible this was a purposeful omission and he had another plan (or maybe not :-)) |
76e746f
to
66d0b08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an ObjectDisposedException that occurs when calling the /admin/host/resume
endpoint by preventing the registration of a DI child scope container for this specific request type.
Key changes:
- Adds detection for admin resume requests to avoid DI container disposal issues
- Updates tests to use sequential job host restart configuration and adds error logging validation
- Implements conditional middleware registration to skip problematic DI scope handling for resume requests
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
HttpRequestExtensions.cs | Adds IsAdminResumeRequest() method to detect resume endpoint requests |
WebJobsApplicationBuilderExtension.cs | Conditionally registers ScriptHostRequestServiceProviderMiddleware to skip resume requests |
DrainModeResumeEndToEndTests.cs | Adds error logging validation and configures sequential job host restart for testing |
test/WebJobs.Script.Tests.Integration/WebHostEndToEnd/DrainModeResumeEndToEndTests.cs
Show resolved
Hide resolved
test/WebJobs.Script.Tests.Integration/WebHostEndToEnd/DrainModeResumeEndToEndTests.cs
Show resolved
Hide resolved
Updated approach after talking with @fabiocav -- we'll just skip the service replacement on /admin/host/resume calls |
Fixes #11263
The bug here is that during a request, we register a DI child scope container with the request... then we dispose it mid-request, so when anything in ASP.NET tries to get a service later, it throws. It's a race b/c it's possible that the request returns before the old container is disposed. Despite this, everything seems to be working b/c all the hard work has already been done by the time the exception is thrown. The JobHost is restarted and everything continues on happily even though we return a 500 to the caller.
I have a preliminary PR that I've just pushed up but it's not truly complete. It'll work for what we need but it's quite tricky to get right with how DI works.
Some background:
azure-functions-host/src/WebJobs.Script.WebHost/DependencyInjection/ScopedResolver.cs
Lines 33 to 37 in b079776
What I've got should be good enough for the very specific scenario we need it for, but there still seems like there's gaps that may bite us later.Want to discuss with @fabiocav later whether we should use this same approach? Or abandon it and just skip registering services for /admin calls? Or if there's other approaches.Will leave it in Draft for now.Edit:
This is now ready for review. Rather than use the non-optimal approach with the built-in DI, we're going to just skip replacing these services on calls that we know don't need them and can be problematic. We can add to this if we need to in the future.
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-proc
branch to be included in Core Tools and non-Flex deployments.in-proc
branch is not requiredrelease_notes.md