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

Fix Signal::CHLD.reset not clearing previously set handler, and fix related race condition in spec #7409

Merged
merged 2 commits into from
Feb 11, 2019

Conversation

asterite
Copy link
Member

Fixes #7243

I thought about many ways to fix the race condition in this spec... I think the only safe alternative is to use a channel and send that over to the signal handler. If the handler runs before we send the channel, it will wait. If it runs later, the process will be already finished.

The only problem with this spec is that if the CHLD handler doesn't run for some reason (bug) then the spec will wait forever in chan.send because nobody will consume it. However, given that everything is working well now I don't this as a big problem, plus signal handling code doesn't change that frequently.

@asterite asterite added kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:specs labels Feb 10, 2019
Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Good catch!

@asterite asterite force-pushed the bug/fix-signal-spec branch from ae5d148 to c606ab5 Compare February 10, 2019 18:02
@asterite asterite force-pushed the bug/fix-signal-spec branch from c606ab5 to 0638446 Compare February 10, 2019 18:56
@asterite asterite changed the title Specs: fix CHLD signal handler spec that had a race condition Fix Signal::CHLD.reset not clearing previously set handler, and fix related race condition in spec Feb 10, 2019
@asterite
Copy link
Member Author

Okay... my first commit was missing Signal::CHLD.reset to clear that handler, because later if another spec did Process.new(...) and that process ended then that same handler would be called, hanging everything (on chan.receive), which effectively happened.

I added Signal::CHLD.reset and ran the specs locally... only to find out that that didn't work. The intention of keeping a default handler when Signal::CHLD.reset was good, but any previously set handler wasn't cleared. So this PR also fixes that. Phew!

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Oops, I completely overlooked that. Thanks for fixing this 🙇

@sdogruyol sdogruyol merged commit df0fae8 into crystal-lang:master Feb 11, 2019
@sdogruyol sdogruyol added this to the 0.28.0 milestone Feb 11, 2019
@asterite asterite deleted the bug/fix-signal-spec branch March 30, 2019 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:specs topic:stdlib:runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rarely failing nil assertion in signal_spec
3 participants