Provide fix for #81093 - "Mono does not emit ProcessExit event on SIGTERM" #100056
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…on SIGTERM"
* src/mono/mono/mini/mini-posix.c
- Add signal handler for SIGTERM
* src/mono/mono/mini/mini-windows.c
- Add signal handler for SIGTERM
- Use the correct signal for handler
* src/mono/mono/mini/mini-runtime.c
- Add mono_sigterm_signal_handler to process SIGTERM that will set a global variable
to be monitored by the GC finalizer thread
- Set a default exit code before setting the term_signaled variable that gets checked in gc
* src/mono/mono/mini/mini-runtime.h
- Define prototype for mono_sigterm_signal_handler()
* src/mono/mono/metadata/gc.c
- Monitor for sigterm and kick off the shutdown process when encountered by calling mono_runtime_try_shutdown().
- Exit with either the user set exitcode (System.Environment.ExitCode) or SIGTERM + 128.
- Simplify use of exit code now that a default is being set
- Rename term_signaled to match mono style
- Remove volatile attribute
- Move testing of shutdown until after the sem wait
* src/libraries/System.Runtime/tests/System/ExitCodeTests.Unix.cs
- Re-enable ExitCodeTests for mono
* src/mono/mono/mini/exceptions-amd64.c
src/mono/mono/mini/exceptions-x86.c
- Add control event handling for windows
nealef
requested review from
vargaz,
BrzVlad,
lambdageek,
steveisok and
thaystg
as code owners
dotnet-policy-service
bot
added
the
community-contribution
This was referenced May 22, 2024
|
I cycled CI just to have up to date checks |
This was referenced Jun 13, 2024
kg
reviewed
Jun 13, 2024
| @@ -272,6 +298,11 @@ void win32_seh_set_handler(int type, MonoW32ExceptionHandler handler) | |||
| case SIGSEGV: | |||
| segv_handler = handler; | |||
| break; | |||
| case SIGTERM: | |||
| term_handler = handler; | |||
| if (!SetConsoleCtrlHandler(mono_win_ctrl_handler, TRUE)) | |||
There was a problem hiding this comment.
This adds a new handler, so if win32_seh_set_handler was previously called to install a different term_handler, shouldn't we uninstall the old one first?
There was a problem hiding this comment.
actually, i misread, but i think this would add a copy of our handler each time you set a term handler, so we might end up with 3 copies of the same one installed unless the kernel removes duplicates. the docs don't say whether it does
if an application calls AllocConsole or AttachConsole this handler will get uninstalled, but that's probably fine to consider as a 'don't do that' problem
|
lgtm but i'm not an expert on our EH internals or signals |
This was referenced Jul 11, 2024
Open
This was referenced Jul 11, 2024
|
/ba-g all dead lettering on some tests, signed off by various managers & engineers who know relevant areas |
akoeplinger
added a commit
to dotnet/sdk
that referenced
this pull request
Jul 17, 2024
The mono issue that caused the different exit code was fixed in dotnet/runtime#100056. We now have the same exit code as coreclr so remove the mono special case. Fixes dotnet/source-build#3174 Fixes dotnet/source-build#4514
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supersedes #82813
Fixes #81093
src/mono/mono/mini/mini-posix.c
src/mono/mono/mini/mini-windows.c
src/mono/mono/mini/mini-runtime.c
src/mono/mono/mini/mini-runtime.h
src/mono/mono/metadata/gc.c
src/libraries/System.Runtime/tests/System/ExitCodeTests.Unix.cs
src/mono/mono/mini/exceptions-amd64.c src/mono/mono/mini/exceptions-x86.c
Note: The comments in the original PR regarding possible memory barriers and acquire/release are still to be addressed.