Change how ANCM recycles app#52807
Conversation
872f421 to
8c53287
Compare
amcasey
left a comment
There was a problem hiding this comment.
Your explanation makes sense and your code seems to match your explanation. I had a bunch of questions and suggestions though.
| void StartShutdown() | ||
| { | ||
| // Shutdown has already been started | ||
| if (m_shutdown.joinable()) |
There was a problem hiding this comment.
Does this stay true once the thread completes?
There was a problem hiding this comment.
It will be true if the thread is still running or if the thread completed but hasn't had .join() called on it.
There was a problem hiding this comment.
I assume some external factor preverts StartShutdown from being called again after the thread has been joined?
There was a problem hiding this comment.
Yeah, both places StartShutdown is called check g_fInShutdown which is set from the thread in StartShutdown. I think putting a check for g_fInShutdown in StartShutdown would be a good idea though even if it's redundant.
| void ShimOptions::SetShutdownDelay(const std::wstring& shutdownDelay) | ||
| { | ||
| auto millsecondsValue = std::stoi(shutdownDelay); | ||
| if (millsecondsValue < 0) |
There was a problem hiding this comment.
I'm not an expert but does a delay of 0 in the sleep_for below behave differently than a positive non-zero delay?
Edit: Actually, it looks like 0 might mean fallback to the old behavior?
There was a problem hiding this comment.
Yeah 0 is fallback. Also 0 in sleep_for will just return and not sleep.
amcasey
left a comment
There was a problem hiding this comment.
Sorry, I lost track of this one. Thanks for the changes!
| if (fServerInitiated) | ||
| // Ignore fServerInitiated for IISExpress | ||
| // Recycle doesn't do anything in IISExpress, we need to explicitly shutdown | ||
| if (m_pHttpServer.IsCommandLineLaunch()) |
There was a problem hiding this comment.
nit: if this is the canonical way of checking whether we're under IISExpress, consider making it a helper function somewhere.
There was a problem hiding this comment.
I can't confidently say it's the check for IISExpress. But that seems to be what we do elsewhere in the project.
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/8789089434 |
Fix for #41340
Issue summary:
When an app is recycling (config change, new deployment, regularly scheduled recycle, resource utilization rule recycle, etc.) IIS will notify our native module and we will initiate a shutdown of the managed application. While we're shutting down we reject any new requests we get with a 503. IIS has a feature (enabled by default) called overlapped recycle, where it will start a new instance of the application while the old one is shutting down, and it will queue then send incoming requests to the new app. In ASP.NET this worked well and 503's would not occur during overlapped recycles. In ASP.NET Core this didn't work and incoming requests weren't being sent to the new app even though it was started.
Fix summary:
I discovered that if you blocked the
OnGlobalStopListeningmethod, which is how IIS tells us it will stop sending us requests, then we will continue to get new requests until we complete the method. And since we were calling shutdown inline in response to that method being called we were blocking until the managed app shutdown and during shutdown we would reject incoming requests with 503. So the fix was to run shutdown on a thread and exitOnGlobalStopListeningASAP. This lets IIS stop sending us requests and queue and send them to the new app while we shut down the managed application.The fix also includes a default delay of 1 second before we call shutdown in order to reduce any races between IIS actually stopping sending us requests after
OnGlobalStopListeningcompletes and us starting shut down which is when we would start sending 503's. The delay is configurable viashutdownDelayin the web config, orANCM_shutdownDelayenvironment variable (both in milliseconds). I'm sort of leaning towardsdelayShutdownas a name instead, we can bike shed on that 😃There is a second fix that was found while trying to fix the original issue. By listening to
GL_APPLICATION_STOPwe can now properly detect "Apps with preload + always running that don't receive a request before recycle/shutdown" and IISExpress app shutdown. Issue #28089 and can't find the one about IISExpress 🤷