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

test_handle_interrupted? may expose improper handle_interrupt logic #5314

Closed
headius opened this issue Sep 18, 2018 · 4 comments
Closed

test_handle_interrupted? may expose improper handle_interrupt logic #5314

headius opened this issue Sep 18, 2018 · 4 comments

Comments

@headius
Copy link
Member

headius commented Sep 18, 2018

The test test_handle_interrupted? may fail at test/mri/ruby/test_thread.rb:862 due to flaws in Thread#handle_interrupt logic. If the handling is allowing exceptions to be thrown that should be masked, it could trigger push of :ng1 earlier in the test, causing a sporadic failure as seen in this job: https://travis-ci.org/jruby/jruby/jobs/430135641

I am quarantining this test until we can determine if it is a flawed test or a bug in handle_interrupt.

FWIW I have stepped through handle_interrupt and been unable to reproduce the issue locally, so it may involve more complicated timing than can be simulated in a debug session.

@headius
Copy link
Member Author

headius commented Sep 18, 2018

Other tests have been masked for this reason; it seems there's a bug in handle_interrupt that should be fixed before 9.2.1.

@headius headius reopened this Sep 18, 2018
@headius headius added this to the JRuby 9.2.1.0 milestone Sep 18, 2018
@larskanis
Copy link

The reason for ng1 in the test is probably that the interrupt mask is currently not inherited to new threads in JRuby and Truffleruby. In MRI it's done here.

Here is a minimal test case. In JRuby and Truffleruby this aborts the thread, in MRI this print :ok after one second:

Thread.handle_interrupt(RuntimeError=>:never){ Thread.new{ sleep 1; p :ok } }.raise

Hope this can be fixed in 9.2.1.

@headius
Copy link
Member Author

headius commented Oct 9, 2018

@larskanis Ah, thanks for the pointer, that makes sense. Should be an easy one-liner...I'll give it a shot.

headius added a commit that referenced this issue Oct 9, 2018
This fixes the original failure that triggered #5314 (just the
test_handle_interrupted? test) but not the other cases that were
already excluded here.
@headius
Copy link
Member Author

headius commented Oct 9, 2018

I fixed the case that @larskanis explained, and test_handle_interrupted? does appear to pass consistently locally. The other three excluded tests still don't pass, though, and need further investigation.

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

No branches or pull requests

2 participants