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

Kernel#sleep swallows InterruptedException #4206

Closed
raelik opened this issue Oct 5, 2016 · 9 comments
Closed

Kernel#sleep swallows InterruptedException #4206

raelik opened this issue Oct 5, 2016 · 9 comments

Comments

@raelik
Copy link

@raelik raelik commented Oct 5, 2016

Currently, the Kernel#sleep code silently swallows any InterruptedException thrown by the native Java Thread.sleep(). Presumably, this was done in an attempt to prevent "spurious wakeups" from prematurely waking up sleeping threads. @headius has an old gist out there that was intended to remove this (https://gist.github.com/headius/5393700), but it simply gets rid of the entire spin lock and just calls the native sleep(). I think the spin lock is still necessary to account for spurious wakeups, but it shouldn't catch the InterruptedException, as those are only generated by Thread.interrupt(). According to the Java documentation, a spurious wakeup it truly spurious: it generates no exception or timeout, the sleeping thread simply stops sleeping.

I think a good solution would be to change the spin lock code in RubyKernel#sleep (currently starting at line 660) to this:

        // Spurious wakeup-loop
        do {
            long loopStartTime = System.currentTimeMillis();
            // We break if we know this sleep was NOT explicitly woken up/interrupted
            if (rubyThread.sleep(milliseconds)) break;
            milliseconds -= (System.currentTimeMillis() - loopStartTime);
        }
        while (milliseconds > 0);

The original code wraps a try/catch around the sleep, which is what swallows the InterruptedException. Also, it breaks if sleep returns false... which seems to actually do the opposite of the intended behavior. RubyThread#sleep returns false if the sleep was bounded and did not sleep for the correct amount of time (i.e. spurious wakeup). Breaking from the while loop here when that happens would allow the spurious wakeup to terminate the sleep early. A spurious wakeup should not cause the sleep to terminate early, and an InterruptedException should bubble up, since those are quite intentional.

@headius
Copy link
Member

@headius headius commented Oct 6, 2016

I think you're right. This also was reported in another bug...I'll try to find that.

@headius
Copy link
Member

@headius headius commented Oct 6, 2016

The InterruptedException is checked, so it still needs to be caught...but we'll just break out of the spin loop if it is raised. I have a fix coming...writing up a test now.

@raelik
Copy link
Author

@raelik raelik commented Oct 6, 2016

I guess that'll work, instead of trying to rescue InterruptedException, I can just check the native thread for isInterrupted() right after the sleep exits if I need to do something specific in that case.

@headius
Copy link
Member

@headius headius commented Oct 6, 2016

Yeah sorry...we can't really allow the InterruptedException to get out, and there's no equivalent exception that MRI throws here for interrupted sleeps. The closest equivalent behavior in Ruby would be Thread#wakeup, which interrupts the sleep but does not raise an error.

@raelik
Copy link
Author

@raelik raelik commented Oct 7, 2016

Right, that's exactly what I realized after I read that 👍

@Pr0methean
Copy link

@Pr0methean Pr0methean commented Aug 21, 2019

If the user needs to check isInterrupted() before returning, that should probably be done in a wrapper method of the same class for convenience (even if the wrapped method also needs to be public). Compare this method from Guava Testlib: https://github.com/google/guava/blob/794a10a4495fd7cd554eb0a9e480708d0277aced/guava/src/com/google/common/util/concurrent/Uninterruptibles.java#L382-L402

@headius
Copy link
Member

@headius headius commented Aug 21, 2019

@Pr0methean Yeah I agree, it would be better to have an explicit uninterruptible sleep, but we're somewhat limited by what CRuby supports here. All the publicly callable sleep functions go through logic that checks for spurious wakeup and goes back to sleep. They do have C functions that will allow spurious wakeup, but as far as I can tell this is only used for utilitarian sleeps like in between polling a pid for completion.

headius added a commit to headius/jruby that referenced this issue Aug 22, 2019
In order for users to be able to test if the thread's sleep loop
was interrupted, we need to ensure the interrupt bit is set after
an interrupted loop, even if the loop itself ignores the
interrupt.

See jruby#4206.
@headius
Copy link
Member

@headius headius commented Aug 22, 2019

@Pr0methean I've pushed a patch in #5845 that re-interrupts the thread after the loop. I do have concerns though, since this sends the thread on its merry way with an interrupt bit just waiting to strike. Do you know if there's some written justification for this re-interrupting logic when doing a spin loop for uninterruptible sleep?

@Pr0methean
Copy link

@Pr0methean Pr0methean commented Aug 22, 2019

So that the next interruptible operation will handle the interrupt.

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.