-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix: always close signal handler pipe in fork/exec #6060
Fix: always close signal handler pipe in fork/exec #6060
Conversation
We must always close the signal handler pipe after between a fork/exec combination, otherwise a simple `system("")` call prevents further signals from being handled. fixes crystal-lang#5830
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels pretty hacky, having two different sets of hooks depending on if we fork+exec or just fork. I can't think of any better designs though. We could always run the after-fork callbacks before we exec but that's reduce exec performance by a lot. So I guess this is kind of an optimization - so the extra code is somewhat justified.
Hi, Signal::INT.trap do
puts "Canceled by user"
exit
end
puts "blocking before: #{STDIN.blocking}"
system ""
puts "blocking after: #{STDIN.blocking}"
puts "Press CTRL+C"
gets Output:
A call to Signal::INT.trap do
puts "Canceled by user"
exit
end
system ""
STDIN.blocking = false
puts "Press CTRL+C"
gets Output:
|
Good grief, my patch doesn't work anymore 😭 @forksaber ohhh, that's interesting! |
Actually, STDIN, STDOUT and STDERR all got their blocking state changed. Since signals are handled through the event loop, but |
We should probably do what @RX14 said once: let's not change the blocking state of STDIN, STDOUT and STDERR. Yeah, they will be blocking, but we'll get rid of a ton of problems. Later when threads are implemented we can revisit the issue. |
I'm closing this PR as it doesn't solve anything: the issue is that we can't set O_NONBLOCK on STDIN, STDOUT and STDERR and can't expect them to not be changed by other processes. |
We must always close the signal handler pipe between a fork/exec combination, otherwise a simple
system("")
call prevents further signals from being handled.fixes #5830