Enable mono runtime to handle SIGTERM like CoreCLR #82806

[nealef]

Provide fix for #81093 - "Mono does not emit ProcessExit event 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

• 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

• 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.

[vargaz]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

[nealef]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

[tmds]

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

It doesn't.

The user code may prevent the application from terminating, for example by blocking the ProcessExit event.

SIGTERM is a friendly request for an app to terminate.

If it doesn't terminate in a timely fashion, the user can send it SIGKILL.

That's what a control process, like systemd will do.

@nealef can you remove the following line, which skips the SigTermExitCode test on Mono:

runtime/src/libraries/System.Runtime/tests/System/ExitCodeTests.Unix.cs

Line 19 in db21974

[ActiveIssue("https://github.com/dotnet/runtime/issues/31656",TestRuntimes.Mono)]

[tmds]

The SigTermExitCode test is failing, because Environment.ExitCode changes aren't getting picked up.

I think the fix may be something like: call mono_environment_exitcode_set(128+SIGTERM) at the start of mono_sigterm_signal_handler, and call exit(mono_environment_exitcode_get() after mono_runtime_try_shutdown.

[nealef]

Fixed

[tmds]

Thanks.

Let's see if the test passes for all cases with these changes.

[tmds]

The test is passing for all cases. So it has the right behavior now concerning the ExitCode.

I'm not familiar enough with mono to approve the implementation.

[lambdageek]

LGTM. couple minor nits.

[lambdageek]

Not sure what's up with (Build mono minijit Pri0 Runtime Tests Run windows x64 release) - might be related?

ping @lateralusX

[lateralusX]

If this is going to work on Windows we also need to handle that signal in win32_seh_set_handler as well as react to the exception that will be generated and passed to seh_vectored_exception_handler.

[nealef]

Adding the case SIGTERM and the corresponding term_handler variable in exceptions_[x86|amd64] appears straightforward but how are they then used? I see there is exception handling for EXCEPTION_ILLEGAL_INSTRUCTION etc. where the handler is picked up and called, but how and where would this particular signal be fielded?

[nealef]

@lateralusX@lambdageek Looking for further guidance on the outstanding Windows' issue.

[lateralusX]

On windows there is no direct mapping with "SIGTERM", the closest we have is the probably SetConsoleCtrlHandler and mapping different scenarios like done by:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs

Line 70 in 2327a36

privatestaticInterop.BOOLHandlerRoutine(intdwCtrlType)

This is how its handled by CoreCLR:

runtime/src/coreclr/vm/ceemain.cpp

Line 605 in ffffc4b

::SetConsoleCtrlHandler(DbgCtrlCHandler, TRUE/*add*/);

So for Mono Windows handling of "SIGTERM" it probably need to setup a console ctrl handler and then react on the event as part of that handler and can't be handled through existing vectorized exception handling logic.

[lateralusX]

Since this is updated from a different thread without any locking, maybe we would need a read memory barrier to make sure we see updated value, especially when wait == FALSE,. There might be one hidden in the mono_coop_sem_timedwait, looks like at least sem_trywait seems to issue memory barriers, and other implementations probably do as well (like WaitForSingleObjectEx), so maybe not needed in the end, but we depend on implementation details. I guess we can leave it as is for now, just wanted to make a comment/note around potential but adding a read barrier in that code path shouldn't be too dramatic.

[tmds]

I think that at one point, this PR had mono_term_signaled as volatile.

Maybe that (or something else) is needed to ensure these lines don't reorder, causing the wrong exit code to be picked up.

This is how the exit code is set:

mono_environment_exitcode_set(128+SIGTERM);	/* Set default exit code */
mono_term_signaled = TRUE;

[lateralusX]

We can always do acquire/release semantics on mono_term_signaled and we would make sure this is not reordered by compiler or CPU. Just using volatile on mono_term_signaled will only prevent compiler to reorder load/store but CPU is still free to do load/store reorder.

[nealef]

Closing this as a new PR was generated that includes Windows has been created: #100056

[nealef]

Provide fix for #81093 - "Mono does not emit ProcessExit event 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

• 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

• 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.

[vargaz]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

[nealef]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

[tmds]

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

It doesn't.

The user code may prevent the application from terminating, for example by blocking the ProcessExit event.

SIGTERM is a friendly request for an app to terminate.

If it doesn't terminate in a timely fashion, the user can send it SIGKILL.

That's what a control process, like systemd will do.

@nealef can you remove the following line, which skips the SigTermExitCode test on Mono:

runtime/src/libraries/System.Runtime/tests/System/ExitCodeTests.Unix.cs

Line 19 in db21974

[ActiveIssue("https://github.com/dotnet/runtime/issues/31656",TestRuntimes.Mono)]

[tmds]

The SigTermExitCode test is failing, because Environment.ExitCode changes aren't getting picked up.

I think the fix may be something like: call mono_environment_exitcode_set(128+SIGTERM) at the start of mono_sigterm_signal_handler, and call exit(mono_environment_exitcode_get() after mono_runtime_try_shutdown.

[nealef]

Fixed

[tmds]

Thanks.

Let's see if the test passes for all cases with these changes.

[tmds]

The test is passing for all cases. So it has the right behavior now concerning the ExitCode.

I'm not familiar enough with mono to approve the implementation.

[lambdageek]

LGTM. couple minor nits.

[lambdageek]

Not sure what's up with (Build mono minijit Pri0 Runtime Tests Run windows x64 release) - might be related?

ping @lateralusX

[lateralusX]

If this is going to work on Windows we also need to handle that signal in win32_seh_set_handler as well as react to the exception that will be generated and passed to seh_vectored_exception_handler.

[nealef]

Adding the case SIGTERM and the corresponding term_handler variable in exceptions_[x86|amd64] appears straightforward but how are they then used? I see there is exception handling for EXCEPTION_ILLEGAL_INSTRUCTION etc. where the handler is picked up and called, but how and where would this particular signal be fielded?

[nealef]

@lateralusX@lambdageek Looking for further guidance on the outstanding Windows' issue.

[lateralusX]

On windows there is no direct mapping with "SIGTERM", the closest we have is the probably SetConsoleCtrlHandler and mapping different scenarios like done by:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs

Line 70 in 2327a36

privatestaticInterop.BOOLHandlerRoutine(intdwCtrlType)

This is how its handled by CoreCLR:

runtime/src/coreclr/vm/ceemain.cpp

Line 605 in ffffc4b

::SetConsoleCtrlHandler(DbgCtrlCHandler, TRUE/*add*/);

So for Mono Windows handling of "SIGTERM" it probably need to setup a console ctrl handler and then react on the event as part of that handler and can't be handled through existing vectorized exception handling logic.

[lateralusX]

Since this is updated from a different thread without any locking, maybe we would need a read memory barrier to make sure we see updated value, especially when wait == FALSE,. There might be one hidden in the mono_coop_sem_timedwait, looks like at least sem_trywait seems to issue memory barriers, and other implementations probably do as well (like WaitForSingleObjectEx), so maybe not needed in the end, but we depend on implementation details. I guess we can leave it as is for now, just wanted to make a comment/note around potential but adding a read barrier in that code path shouldn't be too dramatic.

[tmds]

I think that at one point, this PR had mono_term_signaled as volatile.

Maybe that (or something else) is needed to ensure these lines don't reorder, causing the wrong exit code to be picked up.

This is how the exit code is set:

mono_environment_exitcode_set(128+SIGTERM);	/* Set default exit code */
mono_term_signaled = TRUE;

[lateralusX]

We can always do acquire/release semantics on mono_term_signaled and we would make sure this is not reordered by compiler or CPU. Just using volatile on mono_term_signaled will only prevent compiler to reorder load/store but CPU is still free to do load/store reorder.

[nealef]

Closing this as a new PR was generated that includes Windows has been created: #100056

[nealef]

Provide fix for #81093 - "Mono does not emit ProcessExit event 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

• 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

• 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.

[vargaz]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

[nealef]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

[tmds]

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

It doesn't.

The user code may prevent the application from terminating, for example by blocking the ProcessExit event.

SIGTERM is a friendly request for an app to terminate.

If it doesn't terminate in a timely fashion, the user can send it SIGKILL.

That's what a control process, like systemd will do.

@nealef can you remove the following line, which skips the SigTermExitCode test on Mono:

runtime/src/libraries/System.Runtime/tests/System/ExitCodeTests.Unix.cs

Line 19 in db21974

[ActiveIssue("https://github.com/dotnet/runtime/issues/31656",TestRuntimes.Mono)]

[tmds]

The SigTermExitCode test is failing, because Environment.ExitCode changes aren't getting picked up.

I think the fix may be something like: call mono_environment_exitcode_set(128+SIGTERM) at the start of mono_sigterm_signal_handler, and call exit(mono_environment_exitcode_get() after mono_runtime_try_shutdown.

[nealef]

Fixed

[tmds]

Thanks.

Let's see if the test passes for all cases with these changes.

[tmds]

The test is passing for all cases. So it has the right behavior now concerning the ExitCode.

I'm not familiar enough with mono to approve the implementation.

[lambdageek]

LGTM. couple minor nits.

[lambdageek]

Not sure what's up with (Build mono minijit Pri0 Runtime Tests Run windows x64 release) - might be related?

ping @lateralusX

[lateralusX]

If this is going to work on Windows we also need to handle that signal in win32_seh_set_handler as well as react to the exception that will be generated and passed to seh_vectored_exception_handler.

[nealef]

Adding the case SIGTERM and the corresponding term_handler variable in exceptions_[x86|amd64] appears straightforward but how are they then used? I see there is exception handling for EXCEPTION_ILLEGAL_INSTRUCTION etc. where the handler is picked up and called, but how and where would this particular signal be fielded?

[nealef]

@lateralusX@lambdageek Looking for further guidance on the outstanding Windows' issue.

[lateralusX]

On windows there is no direct mapping with "SIGTERM", the closest we have is the probably SetConsoleCtrlHandler and mapping different scenarios like done by:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs

Line 70 in 2327a36

privatestaticInterop.BOOLHandlerRoutine(intdwCtrlType)

This is how its handled by CoreCLR:

runtime/src/coreclr/vm/ceemain.cpp

Line 605 in ffffc4b

::SetConsoleCtrlHandler(DbgCtrlCHandler, TRUE/*add*/);

So for Mono Windows handling of "SIGTERM" it probably need to setup a console ctrl handler and then react on the event as part of that handler and can't be handled through existing vectorized exception handling logic.

[lateralusX]

Since this is updated from a different thread without any locking, maybe we would need a read memory barrier to make sure we see updated value, especially when wait == FALSE,. There might be one hidden in the mono_coop_sem_timedwait, looks like at least sem_trywait seems to issue memory barriers, and other implementations probably do as well (like WaitForSingleObjectEx), so maybe not needed in the end, but we depend on implementation details. I guess we can leave it as is for now, just wanted to make a comment/note around potential but adding a read barrier in that code path shouldn't be too dramatic.

[tmds]

I think that at one point, this PR had mono_term_signaled as volatile.

Maybe that (or something else) is needed to ensure these lines don't reorder, causing the wrong exit code to be picked up.

This is how the exit code is set:

mono_environment_exitcode_set(128+SIGTERM);	/* Set default exit code */
mono_term_signaled = TRUE;

[lateralusX]

We can always do acquire/release semantics on mono_term_signaled and we would make sure this is not reordered by compiler or CPU. Just using volatile on mono_term_signaled will only prevent compiler to reorder load/store but CPU is still free to do load/store reorder.

[nealef]

Closing this as a new PR was generated that includes Windows has been created: #100056

[nealef]

Provide fix for #81093 - "Mono does not emit ProcessExit event 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

• 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

• 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.

[vargaz]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

[nealef]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

[tmds]

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

It doesn't.

The user code may prevent the application from terminating, for example by blocking the ProcessExit event.

SIGTERM is a friendly request for an app to terminate.

If it doesn't terminate in a timely fashion, the user can send it SIGKILL.

That's what a control process, like systemd will do.

@nealef can you remove the following line, which skips the SigTermExitCode test on Mono:

runtime/src/libraries/System.Runtime/tests/System/ExitCodeTests.Unix.cs

Line 19 in db21974

[ActiveIssue("https://github.com/dotnet/runtime/issues/31656",TestRuntimes.Mono)]

[tmds]

The SigTermExitCode test is failing, because Environment.ExitCode changes aren't getting picked up.

I think the fix may be something like: call mono_environment_exitcode_set(128+SIGTERM) at the start of mono_sigterm_signal_handler, and call exit(mono_environment_exitcode_get() after mono_runtime_try_shutdown.

[nealef]

Fixed

[tmds]

Thanks.

Let's see if the test passes for all cases with these changes.

[tmds]

The test is passing for all cases. So it has the right behavior now concerning the ExitCode.

I'm not familiar enough with mono to approve the implementation.

[lambdageek]

LGTM. couple minor nits.

[lambdageek]

Not sure what's up with (Build mono minijit Pri0 Runtime Tests Run windows x64 release) - might be related?

ping @lateralusX

[lateralusX]

If this is going to work on Windows we also need to handle that signal in win32_seh_set_handler as well as react to the exception that will be generated and passed to seh_vectored_exception_handler.

[nealef]

Adding the case SIGTERM and the corresponding term_handler variable in exceptions_[x86|amd64] appears straightforward but how are they then used? I see there is exception handling for EXCEPTION_ILLEGAL_INSTRUCTION etc. where the handler is picked up and called, but how and where would this particular signal be fielded?

[nealef]

@lateralusX@lambdageek Looking for further guidance on the outstanding Windows' issue.

[lateralusX]

On windows there is no direct mapping with "SIGTERM", the closest we have is the probably SetConsoleCtrlHandler and mapping different scenarios like done by:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs

Line 70 in 2327a36

privatestaticInterop.BOOLHandlerRoutine(intdwCtrlType)

This is how its handled by CoreCLR:

runtime/src/coreclr/vm/ceemain.cpp

Line 605 in ffffc4b

::SetConsoleCtrlHandler(DbgCtrlCHandler, TRUE/*add*/);

So for Mono Windows handling of "SIGTERM" it probably need to setup a console ctrl handler and then react on the event as part of that handler and can't be handled through existing vectorized exception handling logic.

[lateralusX]

Since this is updated from a different thread without any locking, maybe we would need a read memory barrier to make sure we see updated value, especially when wait == FALSE,. There might be one hidden in the mono_coop_sem_timedwait, looks like at least sem_trywait seems to issue memory barriers, and other implementations probably do as well (like WaitForSingleObjectEx), so maybe not needed in the end, but we depend on implementation details. I guess we can leave it as is for now, just wanted to make a comment/note around potential but adding a read barrier in that code path shouldn't be too dramatic.

[tmds]

I think that at one point, this PR had mono_term_signaled as volatile.

Maybe that (or something else) is needed to ensure these lines don't reorder, causing the wrong exit code to be picked up.

This is how the exit code is set:

mono_environment_exitcode_set(128+SIGTERM);	/* Set default exit code */
mono_term_signaled = TRUE;

[lateralusX]

We can always do acquire/release semantics on mono_term_signaled and we would make sure this is not reordered by compiler or CPU. Just using volatile on mono_term_signaled will only prevent compiler to reorder load/store but CPU is still free to do load/store reorder.

[nealef]

Closing this as a new PR was generated that includes Windows has been created: #100056

[nealef]

Provide fix for #81093 - "Mono does not emit ProcessExit event 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

• 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

• 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.

[vargaz]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

[nealef]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

[tmds]

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

It doesn't.

The user code may prevent the application from terminating, for example by blocking the ProcessExit event.

SIGTERM is a friendly request for an app to terminate.

If it doesn't terminate in a timely fashion, the user can send it SIGKILL.

That's what a control process, like systemd will do.

@nealef can you remove the following line, which skips the SigTermExitCode test on Mono:

runtime/src/libraries/System.Runtime/tests/System/ExitCodeTests.Unix.cs

Line 19 in db21974

[ActiveIssue("https://github.com/dotnet/runtime/issues/31656",TestRuntimes.Mono)]

[tmds]

The SigTermExitCode test is failing, because Environment.ExitCode changes aren't getting picked up.

I think the fix may be something like: call mono_environment_exitcode_set(128+SIGTERM) at the start of mono_sigterm_signal_handler, and call exit(mono_environment_exitcode_get() after mono_runtime_try_shutdown.

[nealef]

Fixed

[tmds]

Thanks.

Let's see if the test passes for all cases with these changes.

[tmds]

The test is passing for all cases. So it has the right behavior now concerning the ExitCode.

I'm not familiar enough with mono to approve the implementation.

[lambdageek]

LGTM. couple minor nits.

[lambdageek]

Not sure what's up with (Build mono minijit Pri0 Runtime Tests Run windows x64 release) - might be related?

ping @lateralusX

[lateralusX]

If this is going to work on Windows we also need to handle that signal in win32_seh_set_handler as well as react to the exception that will be generated and passed to seh_vectored_exception_handler.

[nealef]

Adding the case SIGTERM and the corresponding term_handler variable in exceptions_[x86|amd64] appears straightforward but how are they then used? I see there is exception handling for EXCEPTION_ILLEGAL_INSTRUCTION etc. where the handler is picked up and called, but how and where would this particular signal be fielded?

[nealef]

@lateralusX@lambdageek Looking for further guidance on the outstanding Windows' issue.

[lateralusX]

On windows there is no direct mapping with "SIGTERM", the closest we have is the probably SetConsoleCtrlHandler and mapping different scenarios like done by:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs

Line 70 in 2327a36

privatestaticInterop.BOOLHandlerRoutine(intdwCtrlType)

This is how its handled by CoreCLR:

runtime/src/coreclr/vm/ceemain.cpp

Line 605 in ffffc4b

::SetConsoleCtrlHandler(DbgCtrlCHandler, TRUE/*add*/);

So for Mono Windows handling of "SIGTERM" it probably need to setup a console ctrl handler and then react on the event as part of that handler and can't be handled through existing vectorized exception handling logic.

[lateralusX]

Since this is updated from a different thread without any locking, maybe we would need a read memory barrier to make sure we see updated value, especially when wait == FALSE,. There might be one hidden in the mono_coop_sem_timedwait, looks like at least sem_trywait seems to issue memory barriers, and other implementations probably do as well (like WaitForSingleObjectEx), so maybe not needed in the end, but we depend on implementation details. I guess we can leave it as is for now, just wanted to make a comment/note around potential but adding a read barrier in that code path shouldn't be too dramatic.

[tmds]

I think that at one point, this PR had mono_term_signaled as volatile.

Maybe that (or something else) is needed to ensure these lines don't reorder, causing the wrong exit code to be picked up.

This is how the exit code is set:

mono_environment_exitcode_set(128+SIGTERM);	/* Set default exit code */
mono_term_signaled = TRUE;

[lateralusX]

We can always do acquire/release semantics on mono_term_signaled and we would make sure this is not reordered by compiler or CPU. Just using volatile on mono_term_signaled will only prevent compiler to reorder load/store but CPU is still free to do load/store reorder.

[nealef]

Closing this as a new PR was generated that includes Windows has been created: #100056

[nealef]

Provide fix for #81093 - "Mono does not emit ProcessExit event 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

• 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

• 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.

[vargaz]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

[nealef]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

[tmds]

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

It doesn't.

The user code may prevent the application from terminating, for example by blocking the ProcessExit event.

SIGTERM is a friendly request for an app to terminate.

If it doesn't terminate in a timely fashion, the user can send it SIGKILL.

That's what a control process, like systemd will do.

@nealef can you remove the following line, which skips the SigTermExitCode test on Mono:

runtime/src/libraries/System.Runtime/tests/System/ExitCodeTests.Unix.cs

Line 19 in db21974

[ActiveIssue("https://github.com/dotnet/runtime/issues/31656",TestRuntimes.Mono)]

[tmds]

The SigTermExitCode test is failing, because Environment.ExitCode changes aren't getting picked up.

I think the fix may be something like: call mono_environment_exitcode_set(128+SIGTERM) at the start of mono_sigterm_signal_handler, and call exit(mono_environment_exitcode_get() after mono_runtime_try_shutdown.

[nealef]

Fixed

[tmds]

Thanks.

Let's see if the test passes for all cases with these changes.

[tmds]

The test is passing for all cases. So it has the right behavior now concerning the ExitCode.

I'm not familiar enough with mono to approve the implementation.

[lambdageek]

LGTM. couple minor nits.

[lambdageek]

Not sure what's up with (Build mono minijit Pri0 Runtime Tests Run windows x64 release) - might be related?

ping @lateralusX

[lateralusX]

If this is going to work on Windows we also need to handle that signal in win32_seh_set_handler as well as react to the exception that will be generated and passed to seh_vectored_exception_handler.

[nealef]

Adding the case SIGTERM and the corresponding term_handler variable in exceptions_[x86|amd64] appears straightforward but how are they then used? I see there is exception handling for EXCEPTION_ILLEGAL_INSTRUCTION etc. where the handler is picked up and called, but how and where would this particular signal be fielded?

[nealef]

@lateralusX@lambdageek Looking for further guidance on the outstanding Windows' issue.

[lateralusX]

On windows there is no direct mapping with "SIGTERM", the closest we have is the probably SetConsoleCtrlHandler and mapping different scenarios like done by:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs

Line 70 in 2327a36

privatestaticInterop.BOOLHandlerRoutine(intdwCtrlType)

This is how its handled by CoreCLR:

runtime/src/coreclr/vm/ceemain.cpp

Line 605 in ffffc4b

::SetConsoleCtrlHandler(DbgCtrlCHandler, TRUE/*add*/);

So for Mono Windows handling of "SIGTERM" it probably need to setup a console ctrl handler and then react on the event as part of that handler and can't be handled through existing vectorized exception handling logic.

[lateralusX]

Since this is updated from a different thread without any locking, maybe we would need a read memory barrier to make sure we see updated value, especially when wait == FALSE,. There might be one hidden in the mono_coop_sem_timedwait, looks like at least sem_trywait seems to issue memory barriers, and other implementations probably do as well (like WaitForSingleObjectEx), so maybe not needed in the end, but we depend on implementation details. I guess we can leave it as is for now, just wanted to make a comment/note around potential but adding a read barrier in that code path shouldn't be too dramatic.

[tmds]

I think that at one point, this PR had mono_term_signaled as volatile.

Maybe that (or something else) is needed to ensure these lines don't reorder, causing the wrong exit code to be picked up.

This is how the exit code is set:

mono_environment_exitcode_set(128+SIGTERM);	/* Set default exit code */
mono_term_signaled = TRUE;

[lateralusX]

We can always do acquire/release semantics on mono_term_signaled and we would make sure this is not reordered by compiler or CPU. Just using volatile on mono_term_signaled will only prevent compiler to reorder load/store but CPU is still free to do load/store reorder.

[nealef]

Closing this as a new PR was generated that includes Windows has been created: #100056

[nealef]

Provide fix for #81093 - "Mono does not emit ProcessExit event 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

• 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

• 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.

[vargaz]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

[nealef]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

[tmds]

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

It doesn't.

The user code may prevent the application from terminating, for example by blocking the ProcessExit event.

SIGTERM is a friendly request for an app to terminate.

If it doesn't terminate in a timely fashion, the user can send it SIGKILL.

That's what a control process, like systemd will do.

@nealef can you remove the following line, which skips the SigTermExitCode test on Mono:

runtime/src/libraries/System.Runtime/tests/System/ExitCodeTests.Unix.cs

Line 19 in db21974

[ActiveIssue("https://github.com/dotnet/runtime/issues/31656",TestRuntimes.Mono)]

[tmds]

The SigTermExitCode test is failing, because Environment.ExitCode changes aren't getting picked up.

I think the fix may be something like: call mono_environment_exitcode_set(128+SIGTERM) at the start of mono_sigterm_signal_handler, and call exit(mono_environment_exitcode_get() after mono_runtime_try_shutdown.

[nealef]

Fixed

[tmds]

Thanks.

Let's see if the test passes for all cases with these changes.

[tmds]

The test is passing for all cases. So it has the right behavior now concerning the ExitCode.

I'm not familiar enough with mono to approve the implementation.

[lambdageek]

LGTM. couple minor nits.

[lambdageek]

Not sure what's up with (Build mono minijit Pri0 Runtime Tests Run windows x64 release) - might be related?

ping @lateralusX

[lateralusX]

If this is going to work on Windows we also need to handle that signal in win32_seh_set_handler as well as react to the exception that will be generated and passed to seh_vectored_exception_handler.

[nealef]

Adding the case SIGTERM and the corresponding term_handler variable in exceptions_[x86|amd64] appears straightforward but how are they then used? I see there is exception handling for EXCEPTION_ILLEGAL_INSTRUCTION etc. where the handler is picked up and called, but how and where would this particular signal be fielded?

[nealef]

@lateralusX@lambdageek Looking for further guidance on the outstanding Windows' issue.

[lateralusX]

On windows there is no direct mapping with "SIGTERM", the closest we have is the probably SetConsoleCtrlHandler and mapping different scenarios like done by:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs

Line 70 in 2327a36

privatestaticInterop.BOOLHandlerRoutine(intdwCtrlType)

This is how its handled by CoreCLR:

runtime/src/coreclr/vm/ceemain.cpp

Line 605 in ffffc4b

::SetConsoleCtrlHandler(DbgCtrlCHandler, TRUE/*add*/);

So for Mono Windows handling of "SIGTERM" it probably need to setup a console ctrl handler and then react on the event as part of that handler and can't be handled through existing vectorized exception handling logic.

[lateralusX]

Since this is updated from a different thread without any locking, maybe we would need a read memory barrier to make sure we see updated value, especially when wait == FALSE,. There might be one hidden in the mono_coop_sem_timedwait, looks like at least sem_trywait seems to issue memory barriers, and other implementations probably do as well (like WaitForSingleObjectEx), so maybe not needed in the end, but we depend on implementation details. I guess we can leave it as is for now, just wanted to make a comment/note around potential but adding a read barrier in that code path shouldn't be too dramatic.

[tmds]

I think that at one point, this PR had mono_term_signaled as volatile.

Maybe that (or something else) is needed to ensure these lines don't reorder, causing the wrong exit code to be picked up.

This is how the exit code is set:

mono_environment_exitcode_set(128+SIGTERM);	/* Set default exit code */
mono_term_signaled = TRUE;

[lateralusX]

We can always do acquire/release semantics on mono_term_signaled and we would make sure this is not reordered by compiler or CPU. Just using volatile on mono_term_signaled will only prevent compiler to reorder load/store but CPU is still free to do load/store reorder.

[nealef]

Closing this as a new PR was generated that includes Windows has been created: #100056

[nealef]

Provide fix for #81093 - "Mono does not emit ProcessExit event 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

• 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

• 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.

[vargaz]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

[nealef]

Users expect the process to terminate when it's sent a SIGTERM, and adding a handler for that could cause the process to get stuck if the managed code gets stuck etc.

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

[tmds]

What does coreCLR do in this situation? It fields the SIGTERM and invokes managed code. How does it prevent getting stuck?

It doesn't.

The user code may prevent the application from terminating, for example by blocking the ProcessExit event.

SIGTERM is a friendly request for an app to terminate.

If it doesn't terminate in a timely fashion, the user can send it SIGKILL.

That's what a control process, like systemd will do.

@nealef can you remove the following line, which skips the SigTermExitCode test on Mono:

runtime/src/libraries/System.Runtime/tests/System/ExitCodeTests.Unix.cs

Line 19 in db21974

[ActiveIssue("https://github.com/dotnet/runtime/issues/31656",TestRuntimes.Mono)]

[tmds]

The SigTermExitCode test is failing, because Environment.ExitCode changes aren't getting picked up.

I think the fix may be something like: call mono_environment_exitcode_set(128+SIGTERM) at the start of mono_sigterm_signal_handler, and call exit(mono_environment_exitcode_get() after mono_runtime_try_shutdown.

[nealef]

Fixed

[tmds]

Thanks.

Let's see if the test passes for all cases with these changes.

[tmds]

The test is passing for all cases. So it has the right behavior now concerning the ExitCode.

I'm not familiar enough with mono to approve the implementation.

[lambdageek]

LGTM. couple minor nits.

[lambdageek]

Not sure what's up with (Build mono minijit Pri0 Runtime Tests Run windows x64 release) - might be related?

ping @lateralusX

[lateralusX]

If this is going to work on Windows we also need to handle that signal in win32_seh_set_handler as well as react to the exception that will be generated and passed to seh_vectored_exception_handler.

[nealef]

Adding the case SIGTERM and the corresponding term_handler variable in exceptions_[x86|amd64] appears straightforward but how are they then used? I see there is exception handling for EXCEPTION_ILLEGAL_INSTRUCTION etc. where the handler is picked up and called, but how and where would this particular signal be fielded?

[nealef]

@lateralusX@lambdageek Looking for further guidance on the outstanding Windows' issue.

[lateralusX]

On windows there is no direct mapping with "SIGTERM", the closest we have is the probably SetConsoleCtrlHandler and mapping different scenarios like done by:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/PosixSignalRegistration.Windows.cs

Line 70 in 2327a36

privatestaticInterop.BOOLHandlerRoutine(intdwCtrlType)

This is how its handled by CoreCLR:

runtime/src/coreclr/vm/ceemain.cpp

Line 605 in ffffc4b

::SetConsoleCtrlHandler(DbgCtrlCHandler, TRUE/*add*/);

So for Mono Windows handling of "SIGTERM" it probably need to setup a console ctrl handler and then react on the event as part of that handler and can't be handled through existing vectorized exception handling logic.

[lateralusX]

Since this is updated from a different thread without any locking, maybe we would need a read memory barrier to make sure we see updated value, especially when wait == FALSE,. There might be one hidden in the mono_coop_sem_timedwait, looks like at least sem_trywait seems to issue memory barriers, and other implementations probably do as well (like WaitForSingleObjectEx), so maybe not needed in the end, but we depend on implementation details. I guess we can leave it as is for now, just wanted to make a comment/note around potential but adding a read barrier in that code path shouldn't be too dramatic.

[tmds]

I think that at one point, this PR had mono_term_signaled as volatile.

Maybe that (or something else) is needed to ensure these lines don't reorder, causing the wrong exit code to be picked up.

This is how the exit code is set:

mono_environment_exitcode_set(128+SIGTERM);	/* Set default exit code */
mono_term_signaled = TRUE;

[lateralusX]

We can always do acquire/release semantics on mono_term_signaled and we would make sure this is not reordered by compiler or CPU. Just using volatile on mono_term_signaled will only prevent compiler to reorder load/store but CPU is still free to do load/store reorder.

[nealef]

Closing this as a new PR was generated that includes Windows has been created: #100056