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

Remember STDIN, STDOUT and STDERR blocking state when program begins and restore it at the end #5017

Merged
merged 1 commit into from Sep 27, 2017
Merged

Conversation

asterite
Copy link
Member

@asterite asterite commented Sep 22, 2017

Doesn't fix #2713 because if you change the program to do sleep 1 you can see the same bad behaviour. A different approach is needed, but it's still a good idea to restore the original state.

@bew
Copy link
Contributor

@bew bew commented Sep 22, 2017

Just tested, seems to work pretty well!

@@ -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
Copy link
Contributor

@Sija Sija Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest adding FIXME comment to indicate that it'll fail in case of segfault.

Copy link
Contributor

@refi64 refi64 Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth it to also try to restore blocking behavior in case of SIGSEGV, SIGTERM, and SIGINT?

@asterite
Copy link
Member Author

@asterite asterite commented Sep 22, 2017

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
Copy link
Contributor

@RX14 RX14 Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have blocking?

Copy link
Member Author

@asterite asterite Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean doing STDIN.blocking? At that point STDIN blocking state would have changed already.

Copy link
Member Author

@asterite asterite Sep 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(though we can change the logic in the constructor and do it in Crystal.main, I think it's a good idea)

@chocolateboy
Copy link
Contributor

@chocolateboy chocolateboy commented Sep 22, 2017

Just to clarify: will an unclean shutdown (e.g. kill -9) still leave the stdio descriptors in an altered state with this workaround?

@asterite
Copy link
Member Author

@asterite asterite commented Sep 22, 2017

@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...

@chocolateboy
Copy link
Contributor

@chocolateboy chocolateboy commented Sep 22, 2017

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.

@chocolateboy
Copy link
Contributor

@chocolateboy chocolateboy commented Sep 22, 2017

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
end

This could help to detect and diagnose unexpected IO-related side-effects/regressions in the stdlib which might otherwise be obvious without this "fix".

@asterite
Copy link
Member Author

@asterite asterite commented Sep 22, 2017

@chocolateboy But the blocking state is always changed, so that warning will always be printed. I don't think that's a good idea.

@mverzilli mverzilli added this to the Next milestone Sep 25, 2017
@mverzilli
Copy link

@mverzilli mverzilli commented Sep 27, 2017

@asterite should we merge or would you rather wait for another review?

@asterite asterite merged commit 6e0e4e8 into crystal-lang:master Sep 27, 2017
2 checks passed
@asterite
Copy link
Member Author

@asterite asterite commented Sep 27, 2017

Let's merge :-)

@chocolateboy
Copy link
Contributor

@chocolateboy chocolateboy commented Sep 27, 2017

Doesn't fix #2713

FYI: this has inadvertently closed #2713 (GitHub ignored the "Doesn't" :-)

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

Successfully merging this pull request may close these issues.

7 participants