-
Notifications
You must be signed in to change notification settings - Fork 78
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
Use sigaction without SA_RESTART instead? #16
Comments
Regardless of if we choose to expose |
Do you have any idea how this works on Windows? I would like to keep this crate as cross-platform as possible and I kinda dislike platform-specific options. However, if this works differently on *nix and Windows nowadays I think it's reasonable to change the default value of |
It's already in the platform-specific code. I've created a pull request that works for me. I don't know how the Windows version behaves, though. I think we can make the default to the same as Windows version, but modifiable via features. |
Do not change the default, SA_RESTART should be enabled by default. Disabling it breaks all libraries that don’t handle EINTR correctly. If we expose SA_RESTART, we should do it through a feature flag or as an option at runtime. It is unfortunately very common to just assume SA_RESTART is enabled, Rust stdlib did until 2014 and the Go runtime still does. There is nothing equivalent on windows, since the handler runs on its own thread, there is no need to interrupt system calls. |
Closed as stale. |
Currently it uses the
signal
function which translates tosigaction
with the SA_RESTART flag. Most blocking syscalls aren't interrupted by it, e.g. reading from a file descriptor will continue to wait for data after the signal. Setting arunning
flag won't work unless that read returns and the process can check therunning
flag.I encountered this when I was reading inotify events. Ctrl-C didn't stop it until the next event arrived. I'm on Arch Linux x86_64 if it matters.
The change won't be backwards compatible, so perhaps add another feature to opt in?
The text was updated successfully, but these errors were encountered: