Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V2 overwrites SIGCHLD handler without calling previous one #432

Open
mika-fischer opened this issue Dec 2, 2024 · 9 comments
Open

V2 overwrites SIGCHLD handler without calling previous one #432

mika-fischer opened this issue Dec 2, 2024 · 9 comments

Comments

@mika-fischer
Copy link

We're using boost::process in a library to spawn a process. We use RHEL 8, so no pidfd, unfortunately.

This library is used in an executable that spawns its own child processes and thus installs its own SIGCHLD handler.

It seems that in v2, the boost::process / boost::asio signal handler completely overwrites the application signal handler, which leads to havoc, since the main application does not notice its child processes exiting anymore...

Asio docs for basic_signal_set say (https://live.boost.org/doc/libs/1_86_0/doc/html/boost_asio/reference/basic_signal_set.html#boost_asio.reference.basic_signal_set.multiple_registration_of_signals):

The application must not also register a signal handler using functions such as signal() or sigaction().

This makes the use of basic_signal_set unacceptable in a library IMO.

In v1, there was some effort, to call the previous signal handler as well:

if ((sigchld_handler != SIG_DFL) && (sigchld_handler != SIG_IGN))
sigchld_handler(val);

However, there's also use of asio::signal_set in sigchld_service:

boost::asio::signal_set _signal_set{get_io_context(), SIGCHLD};

In any case, we managed to use v1 for our use-case. But I think it's currently impossible to use boost process v2 for our use case...

Do you agree or am I missing something?

Are there any plans to make v2 usable in a library, where full control over signal handlers cannot be assumed?

@klemens-morgenstern
Copy link
Collaborator

Such a plan can be made now, I guess. It could even land in 1.88.

What exactly do you need? Just disable the use of the signal ad build tine? Would you still want async wait?

@mika-fischer
Copy link
Author

The most important step would be if use of ::signal and ::sigaction (and by extension asio::signal_set) could be disabled at compile time. That would make boost::process safe to use in a library.

Then, use of async_wait etc. could give a compile error if there's no pidfd support available and signals are disabled.

However, at least polling the exit status should be possible, but I think it is with running().

Keeping async_wait without pidfd support and without signal handlers would be nice of course, but would probably involve some kind of polling inside boost::process. I could understand if you don't want that.

Thanks for considering!

@mika-fischer
Copy link
Author

Maybe signalfd could also be an option on Linux? It's much older than pidfd... https://man7.org/linux/man-pages/man2/signalfd.2.html

@mika-fischer
Copy link
Author

mika-fischer commented Dec 2, 2024

No, seems like signalfd is not a viable option: https://ldpreload.com/blog/signalfd-is-useless?reposted-on-request

AFAIUI the fd only gets signal notifications if the signal is masked for the normal signal handling, which defeats the purpose for this use-case...

@klemens-morgenstern
Copy link
Collaborator

I think a pipe trick is better. Open a pipe, close the write side om the parent and asynchronously wait for it.

Would you be available for beta testing?

@mika-fischer
Copy link
Author

I think a pipe trick is better. Open a pipe, close the write side om the parent and asynchronously wait for it.

OK, sounds also good.

Would you be available for beta testing?

Yes, but I'll be on vacation after this week until next year.

@klemens-morgenstern
Copy link
Collaborator

Must've been a stressful year if you got 20 days for Weihnachtsurlaub left.

I'll probably get to work in January or Februrary, it won't make the upcoming release (1.87).

@mika-fischer
Copy link
Author

Indeed 😅

Allright, I'll keep an eye out for it and test it then!

@klemens-morgenstern
Copy link
Collaborator

Merry early Christmas: https://github.com/boostorg/process/compare/no-signal-set?expand=1

Well, if it works that is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants