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

Signal handler API has no way to set SA_RESTART #646

Open
ghost opened this issue Dec 30, 2020 · 5 comments
Open

Signal handler API has no way to set SA_RESTART #646

ghost opened this issue Dec 30, 2020 · 5 comments

Comments

@ghost
Copy link

ghost commented Dec 30, 2020

@nomis commented on Oct 29, 2018, 8:14 PM UTC:

I'm handling SIGCHLD using asio while also doing writes to standard output and reads/writes to files.

If I receive SIGCHLD during a read() or write() then it gets interrupted. The simplest way to deal with this is to set SA_RESTART when setting up the signal handler, but there is no support for doing this and it doesn't happen by default.

This issue was moved by chriskohlhoff from boostorg/asio#157.

@ghost
Copy link
Author

ghost commented Dec 30, 2020

@nomis commented on Nov 3, 2018, 10:33 AM UTC:

I'm going to workaround this by using ld --wrap=sigaction to force the use of SA_RESTART and cause a compile error if sigaction or SA_RESTART does not exist.

I'm not convinced that the read() and write() calls used to pass signals over a pipe to the I/O service are implemented safely if another signal interrupts them, because the return value is never checked for EINTR, but this is not easy to test.

@ghost
Copy link
Author

ghost commented Dec 30, 2020

@nomis commented on Nov 4, 2018, 8:10 PM UTC:

It's a bit fragile to wrap sigaction because on some platforms (NetBSD) there's more than one version of it and while the linker is instructed to wrap sigaction, the header files have defined it as __sigaction14. I need to wrap both versions of the function and linking will fail if __sigaction14 does not exist.

It would be a lot easier if asio could be configured to use SA_RESTART.

@vinipsmaker
Copy link
Contributor

SA_RESTART should be the default. Any other default is a bad default. Code that doesn't use UNIX signals will never be written with signals in mind and it won't be prepared for EINTR. And most (all?) code that will handle EINTR will just use a macro such as TEMP_FAILURE_RETRY() that only contributes to code noise.

For signal() (the old way of setting the signal handling disposition), 4.2BSD changed the bad default from non-SA_RESTART to SA_RESTART. Unfortunately you have to explicitly request for SA_RESTART on the new sigaction() (and not the other way around).

vinipsmaker added a commit to vinipsmaker/asio that referenced this issue Feb 8, 2023
The user doesn't have control over 3rd party library code that he uses
in his project. If any code forgets to make use of a macro such as
TEMP_FAILURE_RETRY() in *any* syscall, we have a bug.

Signals that interrupt syscalls are a bad default and BSD acknowledged
that problem when it changed the default signal semantics to SA_RESTART
way back at the release of 4.2BSD. POSIX upstreamed this option through
the use of SA_RESTART, and ASIO should be making use of this flag by
default (hence this commit).

If any code wants the old behavior, it can opt-out of the new default by
defining the macro ASIO_NO_SET_SA_RESTART.

SA_RESTART is only meaningful when we establish a signal handler
(asio_signal_handler), so we skip SA_RESTART altogether when sa_handler
is set to any other value (SIG_DFL).

Fixes chriskohlhoff#646
@vinipsmaker
Copy link
Contributor

vinipsmaker commented Mar 7, 2023

Commit 50ad5e4 closes this issue. It's now possible to set SA_RESTART when establishing a signal handler.

I'm handling SIGCHLD using asio

You might be interested in PR #1258 which will enhance commit 11c6caf enhances support to handle SIGCHLD signals.

@nomis
Copy link
Contributor

nomis commented May 28, 2023

This is now fixed in Asio 1.27.0 / Boost 1.82.0 but I can't close it because GitHub has set the reporter to @ghost instead of me.

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

Successfully merging a pull request may close this issue.

2 participants