Skip to content

Handle windows shutdown event - #41101

Merged
sbomer merged 2 commits into
dotnet:masterfrom
sbomer:dockerStop
Aug 21, 2020
Merged

sbomer merged 2 commits into
dotnet:masterfrom
sbomer:dockerStop

Conversation

@sbomer

@sbomer sbomer commented Aug 20, 2020 •

Copy link
Copy Markdown
Member

By doing a clean EEShutdown, including calling ProcessExit handlers, as suggested by @stephentoub in #36089 (comment). This allows ProcessExit to respond to 'docker stop', which sends CTRL_SHUTDOWN_EVENT to windows containers. I'm doing the same for the logoff event.

Automated testing is challenging since I don't see a documented way to trigger these events (other than triggering a shutdown or running in a container). I tested this locally to confirm that it produces the desired behavior on 'docker stop', and will do more testing for the logoff and shutdown events outside of docker.

#36089 (comment) suggests removing removing if (dwCtrlType == CTRL_CLOSE_EVENT). This would cause ProcessExit to fire on an unhandled Ctrl+C (currently it does not on windows or unix), so I am making the more targeted change for 5.0.

Fixes #36089

By doing a clean EEShutdown, including calling ProcessExit handlers.
This allows ProcessExit to respond to 'docker stop', which sends
CTRLSHUTDOWN_EVENT to windows containers.

Fixes dotnet#36089
@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@sbomer

sbomer commented Aug 20, 2020 •

Copy link
Copy Markdown
Member Author

Actually, this may require more thought for the logoff event. From https://docs.microsoft.com/en-us/windows/console/handlerroutine

When a console application is run as a service, it receives a modified default console control handler. This modified handler does not call ExitProcess when processing the CTRL_LOGOFF_EVENT and CTRL_SHUTDOWN_EVENT signals. This allows the service to continue running after the user logs off.

I assume that calling ProcessExit is ok for CTRL_SHUTDOWN_EVENT because the system will terminate the process shortly even if the handlers don't. But CTRL_LOGOFF_EVENT doesn't necessarily terminate the process, so maybe it would be better not to do an EEShutDown in this case, and instead provide a cancellable API for the logoff? @jkotas

@sbomer sbomer changed the title Handle windows logoff and shutdown events Handle windows shutdown event Aug 20, 2020
@jkotas

jkotas commented Aug 20, 2020

Copy link
Copy Markdown
Member

Sounds reasonable. We should make the fix as scoped as possible.

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sbomer

sbomer commented Aug 21, 2020

Copy link
Copy Markdown
Member Author

I confirmed that it also works as expected during windows shutdown for a service.

@sbomer
sbomer requested a review from vitek-karas August 21, 2020 17:59

@vitek-karas vitek-karas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely agree that for .NET 5 we should make this as small of a change as possible.
The logoff scenario is something we may want to look into for .NET 6 (can you please file an issue for it).

@sbomer
sbomer merged commit 0730613 into dotnet:master Aug 21, 2020
@sbomer

sbomer commented Aug 21, 2020

Copy link
Copy Markdown
Member Author

/backport to release/5.0

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/218953220

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@sbomer
sbomer deleted the dockerStop branch November 3, 2023 18:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AppDomain.ProcessExit is not invoked on docker stop

4 participants