Remember STDIN, STDOUT and STDERR blocking state when program begins and restore it at the end #5017
Conversation
|
Just tested, seems to work pretty well! |
src/crystal/main.cr
Outdated
| @@ -53,6 +58,12 @@ module Crystal | |||
| ex.inspect_with_backtrace STDERR if ex | |||
| STDOUT.flush | |||
| STDERR.flush | |||
|
|
|||
| # Restore blocking state of STDIN, STDOUT and STDERR | |||
There was a problem hiding this comment.
I'd suggest adding FIXME comment to indicate that it'll fail in case of segfault.
There was a problem hiding this comment.
Would it be worth it to also try to restore blocking behavior in case of SIGSEGV, SIGTERM, and SIGINT?
… and restore it at the end
|
Updated to restore the state on segfault. |
|
|
||
| # :nodoc: | ||
| def self.remember_blocking_state | ||
| @@stdin_is_blocking = IO::FileDescriptor.fcntl(0, LibC::F_GETFL) & LibC::O_NONBLOCK == 0 |
There was a problem hiding this comment.
You mean doing STDIN.blocking? At that point STDIN blocking state would have changed already.
There was a problem hiding this comment.
(though we can change the logic in the constructor and do it in Crystal.main, I think it's a good idea)
|
Just to clarify: will an unclean shutdown (e.g. |
|
@chocolateboy No. This will fix the problem in some cases, while in some it won't. Maybe it's better to just forget about this "fix" altogether... |
|
Is there a downside to it compared to the status quo? It seems like a reasonable thing to do regardless of this bug, and it seems to be better than nothing, for now. If every Crystal program needs to do the equivalent of this to work around this issue, then I'd prefer Crystal to do it automatically, at least until the parallelism work lands and matures. It took me months to figure out that this issue was caused by Crystal. For everyone who has diagnosed it and found their way here, there may be many more who haven't, or who have moved on to a different solution. If this "fix" can make this problem go away in many (most?) of those cases, it seems like a net win, for the time being at least, even if it's only a sticking plaster. |
|
One way to allay fears that this might mask unfixed underlying issues could be to (optionally) issue a warning if a file descriptor's state has changed on exit: if STDIN.blocking != @@stdin_is_blocking
STDERR.puts "restoring blocking state of stdin: #{STDIN.blocking} -> #{@@stdin_is_blocking}"
STDIN.blocking = @@stdin_is_blocking
endThis could help to detect and diagnose unexpected IO-related side-effects/regressions in the stdlib which might otherwise be obvious without this "fix". |
|
@chocolateboy But the blocking state is always changed, so that warning will always be printed. I don't think that's a good idea. |
|
@asterite should we merge or would you rather wait for another review? |
|
Let's merge :-) |
Doesn't fix #2713 because if you change the program to do
sleep 1you can see the same bad behaviour. A different approach is needed, but it's still a good idea to restore the original state.