Forward signals to child process with micromamba run #3152

MihaiBSony · 2024-01-24T21:36:18Z

Hi! This MR tries to fix issue #1820, in which signals are not forwarded to the child process. I think this should count as a first draft - in the final version it's probably a good idea to save the state of the signal handlers and restore it once the call is over, instead of just overwriting them. Looking forward to your feedback!

P.S.: I don't know much about Windows, so I didn't even try to handle that case.

jonashaag · 2024-01-25T08:47:24Z

Do have more understanding of why signal forwarding works on Linux and macOS but not Windows? Should this patch be Windows-only instead?

MihaiBSony · 2024-01-25T08:56:12Z

My changes don't touch windows at all (almost all changes are inside a #ifndef _WIN32 guard). Previously, the only signal that was somewhat working on Linux was SIGTERM, because micromamba was adding a signal handler for it (see removed code in the PR), which in turn was stopping the child process with a SIGTERM. This PR attempts to forward all signals to the child, except for SIGCHLD.

MihaiBSony · 2024-01-25T09:10:13Z

The context where I encountered this as an issue is a setup involving SLURM and pytorch-lightning. There I needed the python interpreter to receive the SIGUSR1 signal, but, whenever I ran it as micromamba run -n env python script.py, sending SIGUSR1 to micromamba would just terminate the micromamba process and the child, since that is the default action for SIGUSR1 according to the signal(7) manpage.

While working on the issue I got some inspiration from the source code of the Singularity/Apptainer containerization tool, since micromamba run seems to roughly have a similar purpose as singularity exec. There they seem to handle signals in a similar way as this PR, forwarding them to the child process (if I understood the code correctly). See here.

jonashaag · 2024-01-25T09:33:27Z

Thanks a lot for the context!

We definitely need to add tests for this feature. I guess it could be as simple as using micromamba run to start a Python script that prints whatever signals it receives, and using that output in the test.

That could also help understanding the Windows status quo :)

We save and restore the signal mask and the signal handlers, as well as stop and restart our `default_signal_handler`.

MihaiBSony · 2024-01-31T15:12:21Z

Hi @jonashaag!

I made the code more robust and added unit tests for signal forwarding and process suspension (with CTRL-Z). Unfortunately the code is using a lot of Linux syscalls, so not even the unit tests are usable for windows. Could you take a look at it and maybe tag other people who have more knowledge of signal handling or linux processes? In particular, I'm not sure if the way I'm dealing with the previous signal handler and the signal mask is correct, and if I'm waiting correctly for the child process. Also, it might make sense to move some of the new code to separate functions to keep things tidy.

On the plus side, I already tested the binary in my setup and it works for my use case! 😀

jonashaag

Great work <3

jonashaag · 2024-01-31T15:39:57Z

libmamba/src/core/run.cpp

+            }
+        }
+
+        // Compute the return status mimicking the private logic in reproc++


Can you add a link to that logic as a comment please

I added a link to the main part of the logic, which is here. Do you want a second link for the wpid < 1 part? That would be here

jonashaag · 2024-01-31T15:41:08Z

libmamba/src/core/run.cpp

@@ -442,24 +477,38 @@ namespace mamba
            }

 #ifndef _WIN32
-            MainExecutor::instance().schedule(


Why this change?

I think I forgot this change from one of the experiments I did. I'll revert it.

Do you know why the signal handler was being set with the MainExecutor instead of directly?

I traced this back to 60d4b3a, but still don't know why the signal was being set in a separate thread.

With the new changes, it seems more difficult to do the modifications correctly in a separate thread. The MainExecutor doesn't seem to have functionality to check if a scheduled task has finished, meaning that it wouldn't be possible to check that modifying and restoring the signal masks happen before the end of the run_in_environment call without using some sort of synchronization primitives. At the same time, the signal mask is set on a per-thread basis, so the calls to pthread_sigmask would have to anyhow be done on the main thread. What do you think about leaving this as is?

@wolfv do you recall why?

libmamba/src/core/run.cpp

jonashaag · 2024-01-31T15:44:38Z

libmamba/src/core/run.cpp

@@ -470,12 +519,23 @@ namespace mamba

            ec = reproc::drain(proc, reproc::sink::null, reproc::sink::null);

-            std::tie(status, ec) = proc.stop(opt.stop);
+#ifndef _WIN32
+            status = wait_for_process(pid);


Between the place where we installed the signal handlers up until after this line, is it possible to get any exceptions? If so, should we put the cleanup/restoring code into a finally?

I did not check, but that sounds like a good idea. Unfortunately I'm not sure how to do that in C++

- add link to relevant reproc code - move Unix-only code into `#ifndef _WIN32` block - fix comment - extend suspension unit test sleep time

jonashaag · 2024-01-31T16:58:48Z

libmamba/src/core/run.cpp

+        // Compute the return status mimicing the private logic in reproc++
+        // https://github.com/DaanDeMeyer/reproc/blob/1c07bdbec3f2ecba7125b9499b9a8a77bf9aa8c7/reproc/src/process.posix.c#L455


Suggested change

// Compute the return status mimicing the private logic in reproc++

// https://github.com/DaanDeMeyer/reproc/blob/1c07bdbec3f2ecba7125b9499b9a8a77bf9aa8c7/reproc/src/process.posix.c#L455

// Compute the return status mimicing the private logic in reproc++:

// https://github.com/DaanDeMeyer/reproc/blob/1c07bdbec3f2ecba7125b9499b9a8a77bf9aa8c7/reproc/src/process.posix.c#L471

// https://github.com/DaanDeMeyer/reproc/blob/1c07bdbec3f2ecba7125b9499b9a8a77bf9aa8c7/reproc/src/process.posix.c#L455

Or

Suggested change

// Compute the return status mimicing the private logic in reproc++

// https://github.com/DaanDeMeyer/reproc/blob/1c07bdbec3f2ecba7125b9499b9a8a77bf9aa8c7/reproc/src/process.posix.c#L455

// Compute the return status mimicing the private logic in reproc++

// https://github.com/DaanDeMeyer/reproc/blob/1c07bdbec3f2ecba7125b9499b9a8a77bf9aa8c7/reproc/src/process.posix.c#L465-L478

jonashaag · 2024-01-31T17:07:34Z

What signal handler do we have installed in this loop?

// Set handlers for all signals and save the previous state.

Ie. what are the previous handlers? If this always the fixed list from set_signal_handler? If so, I guess we could simplify the code by making it less generic?

MihaiBSony · 2024-02-05T09:54:23Z

What signal handler do we have installed in this loop?
// Set handlers for all signals and save the previous state.
Ie. what are the previous handlers? If this always the fixed list from set_signal_handler? If so, I guess we could simplify the code by making it less generic?

When used in our code, I think only the so-called default handler is set. If, on the other hand, run_in_environment is used from third-party code (which could be the case, since it's in libmamba?), then we can't make any assumptions about the signals.

Also, there's actually one corner case which I don't handle and which seems impossible to handle without reworking other parts of the code. If someone uses set_signal_handler to set a non-default handler, I can't restore it at the end of the run_in_environment call, and my code just sets the default handler instead.

The so-called signal handlers from set_signal_handler are actually just threads blocking on a sigwait call. We could save the function that is passed as an argument, but if it's third-party code and has any side-effects before getting to sigwait, the behaviour would be pretty hard to predict. This seems to rather be a design flaw in the set_signal_handler functionality, but maybe there's a reason why we don't just use the normal linux signal handlers instead?

jonashaag · 2024-02-05T12:29:46Z

It seems that set_signal_handler was introduced in #1193. Can you please check @MihaiBSony's last comment and let us know if there is a way to simplify the signal handling here? Especially for the run_in_environment API. @adriendelsalle @AntoinePrv

AntoinePrv · 2024-02-05T15:27:11Z

I don't know much about the signal handlers but I know they must be forwarded to python when used in libmambapy.

Forward signals to child process with micromamba run

0b87cf6

MihaiBSony added 3 commits January 25, 2024 10:38

Fix code style

2a69f00

Make signal propagation work more reliably

136773d

We save and restore the signal mask and the signal handlers, as well as stop and restart our `default_signal_handler`.

Add unit tests for signal forwarding

bdc8e2f

Remove print statements and fix code style

a3592d9

jonashaag reviewed Jan 31, 2024

View reviewed changes

Misc improvements

3048692

- add link to relevant reproc code - move Unix-only code into `#ifndef _WIN32` block - fix comment - extend suspension unit test sleep time

jonashaag reviewed Jan 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forward signals to child process with micromamba run #3152

Forward signals to child process with micromamba run #3152

MihaiBSony commented Jan 24, 2024

jonashaag commented Jan 25, 2024

MihaiBSony commented Jan 25, 2024 •

edited

Loading

MihaiBSony commented Jan 25, 2024

jonashaag commented Jan 25, 2024 •

edited

Loading

MihaiBSony commented Jan 31, 2024

jonashaag left a comment

jonashaag Jan 31, 2024

MihaiBSony Jan 31, 2024

jonashaag Jan 31, 2024

MihaiBSony Jan 31, 2024

jonashaag Jan 31, 2024

MihaiBSony Feb 1, 2024

jonashaag Feb 5, 2024 •

edited

Loading

jonashaag Jan 31, 2024

MihaiBSony Jan 31, 2024

jonashaag Jan 31, 2024

jonashaag Jan 31, 2024

jonashaag commented Jan 31, 2024

MihaiBSony commented Feb 5, 2024

jonashaag commented Feb 5, 2024

AntoinePrv commented Feb 5, 2024

		// Compute the return status mimicing the private logic in reproc++
		// https://github.com/DaanDeMeyer/reproc/blob/1c07bdbec3f2ecba7125b9499b9a8a77bf9aa8c7/reproc/src/process.posix.c#L455

Forward signals to child process with micromamba run #3152

Are you sure you want to change the base?

Forward signals to child process with micromamba run #3152

Conversation

MihaiBSony commented Jan 24, 2024

jonashaag commented Jan 25, 2024

MihaiBSony commented Jan 25, 2024 • edited Loading

MihaiBSony commented Jan 25, 2024

jonashaag commented Jan 25, 2024 • edited Loading

MihaiBSony commented Jan 31, 2024

jonashaag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonashaag Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonashaag commented Jan 31, 2024

MihaiBSony commented Feb 5, 2024

jonashaag commented Feb 5, 2024

AntoinePrv commented Feb 5, 2024

MihaiBSony commented Jan 25, 2024 •

edited

Loading

jonashaag commented Jan 25, 2024 •

edited

Loading

jonashaag Feb 5, 2024 •

edited

Loading