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.

Sign up for GitHub

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

Jump to bottom

[Truffle] Threads might not exit sleep state when interrupted #3131

Closed
nirvdrum opened this issue Jul 13, 2015 · 4 comments
Closed

[Truffle] Threads might not exit sleep state when interrupted #3131

nirvdrum opened this issue Jul 13, 2015 · 4 comments
Assignees
@eregon
Milestone
Non-Release

Comments

@nirvdrum
Copy link
Contributor

It's currently possible for a Ruby thread to ignore being awoken. It looks like a race condition of sorts, so you might need to run the following example a few times to see the issue:

THREAD_COUNT = 10

@pool = THREAD_COUNT.times.map { Thread.new { sleep } }

@pool.each { |t| t.run }

@pool.each(&:join)

Some debugging notes:

When a Ruby thread goes to sleep (Kernel#sleep), its sleep status is set to Status.SLEEP [1]. When Thread.run is called, the status is changed to Status.RUN and the Java thread backing the Ruby thread is interrupted [2]. The exception handler checks the status and sets a flag to determine whether to break out of the sleep [3].

Based on some logging I added, the exception handler is seeing the thread's status as Staus.SLEEP, not Status.RUN and as such, it's not setting the shouldWakeUp flag. When the exception is propagated, it's handled in ThreadManager and stops propagating up [4]. At this point, result is null and the action corresponding to the thread sleep is executed again [5]. This is the normal flow of events (i.e., the block is supposed to be executed again), but since the shouldWakeUp flag wasn't properly set, the thread goes to sleep again and interrupt is now lost.

[1] --

jruby/truffle/src/main/java/org/jruby/truffle/runtime/subsystems/ThreadManager.java

Line 122 in b91c9ed

runningThread.setStatus(Status.SLEEP);

[2] --

jruby/truffle/src/main/java/org/jruby/truffle/runtime/core/RubyThread.java

Lines 173 to 176 in 5c8c448

status = Status.RUN;
Thread t = thread;
if (t != null) {
t.interrupt();

[3] --

jruby/truffle/src/main/java/org/jruby/truffle/nodes/core/KernelNodes.java

Lines 1829 to 1831 in b91c9ed

if (thread.getStatus() == Status.RUN) { // Thread#{wakeup,run}
shouldWakeUp = true;
}

[4] -- https://github.com/jruby/jruby/blob/master/truffle/src/main/java/org/jruby/truffle/runtime/subsystems/ThreadManager.java#L134
[5] -- https://github.com/jruby/jruby/blob/master/truffle/src/main/java/org/jruby/truffle/runtime/subsystems/ThreadManager.java#L103-L105
@pitr-ch
Copy link
Member

pitr-ch commented Jul 13, 2015

I think t.run can be executed before the thread goes to sleep, so the thread is never woken up.

@nirvdrum
Copy link
Contributor Author

@pitr-ch But I'm quite clearly seeing it being woken up. The status value is just wrong by then, which is the confusing part. I suppose what could happen is it's woken before it goes to sleep, which sets the status to Status.RUN, the code that sets the status to Status.SLEEP is then run, the Java thread goes to sleep and sees the interrupted status flag is set, triggering the exception handler.

@eregon
Copy link
Member

eregon commented Jul 14, 2015

There is a tension between the script shown and this spec:
https://github.com/jruby/jruby/blob/master/spec/ruby/core/thread/shared/wakeup.rb#L26
Essentially, the spec expects the Thread#run to be lost if invoked before the thread is sleeping (so the thread stays on sleep until line 27 is executed) while the code above does not.

To show the problem in the snippet, you could call
Thread.current.run before sleep and then they would not wake unless some other thread calls run while they are sleeping`

The reason the code above does not deadlock on MRI is just luck because the threads in the pool get in sleep before the main thread continues. A fix to make sure they are sleeping would be:
@pool.each { |t| Thread.pass until t.status == "sleep" }.

I am not sure what is better.

@eregon eregon closed this as completed in dfc5f8a Jul 14, 2015
@eregon
Copy link
Member

eregon commented Jul 14, 2015

I implemented the MRI semantics, so Thread#wakeup/run only wakes up the thread if it is actually sleeping.
We now have a separate flag for wakeUp (and not reusing status) for clarity.
If we change our mind it's easy enough to move to "no wakeup lost" semantics and tag the spec.

@enebo enebo added this to the truffle-dev milestone Jul 22, 2015
@enebo enebo added this to the Non-Release milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@eregon eregon

Labels
None yet
Projects
None yet
Milestone
Non-Release
Development

No branches or pull requests
4 participants
@nirvdrum @enebo @eregon @pitr-ch