Skip to content
Browse files

Remove several locks involved in thread mail and lifecycle.

The deadlock reported in #1015 was caused by RubyThread#dispose
and Thread#kill logic (RubyThread#kill) acquiring the same two
locks in opposite order. dispose first acquired the RubyThread
monitor, in order to read and write volatile state, and then
acquired the ThreadService monitor without releasing RubyThread in
order to unregister the RubyThread object. kill acquired the
ThreadService monitor and then attempted to acquire the RubyThread
monitor in order to deliver the kill event.

As it turned out, the "mail" field on RubyThread was the primary
actor triggering the use of synchronization, and much of that
synchronization was not necessary. The removal of the thread from
ThreadService did not need to be synchronized. The dispose and
receiveMail methods did not need to be synchronized if the mail
slot was made into an atomic reference. The deliverEvent method
did not need to be synchronized for any reason.

I made the appropriate changes to turn mail into an atomic
reference, avoid keeping the RubyThread lock when calling into
ThreadService to remove the RubyThread, and remove locks that
became irrelevant once mail became atomic.

Fixes #1015.
  • Loading branch information...
1 parent bc7621f commit 937f414be083800b78a0aacc6203d2c1eda1f04d @headius headius committed Sep 15, 2013
View
31 core/src/main/java/org/jruby/RubyThread.java
@@ -42,6 +42,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.Map;
import java.util.Set;
@@ -146,13 +147,13 @@
private final AtomicReference<Status> status = new AtomicReference<Status>(Status.RUN);
/** Mail slot for cross-thread events */
- private volatile ThreadService.Event mail;
+ private final AtomicReference<ThreadService.Event> mail = new AtomicReference<ThreadService.Event>();
/** The current task blocking a thread, to allow interrupting it in an appropriate way */
private volatile BlockingTask currentBlockingTask;
/** The list of locks this thread currently holds, so they can be released on exit */
- private final List<Lock> heldLocks = new ArrayList<Lock>();
+ private final List<Lock> heldLocks = Collections.synchronizedList(new ArrayList<Lock>());
/** Whether or not this thread has been disposed of */
private volatile boolean disposed = false;
@@ -178,7 +179,8 @@ public void receiveMail(ThreadService.Event event) {
// if we're already aborting, we can receive no further mail
if (status.get() == Status.ABORTING) return;
- mail = event;
+ // FIXME: this was not checking null before, but maybe it should
+ mail.set(event);
switch (event.type) {
case KILL:
status.set(Status.ABORTING);
@@ -201,9 +203,9 @@ public void receiveMail(ThreadService.Event event) {
}
- public synchronized void checkMail(ThreadContext context) {
- ThreadService.Event myEvent = mail;
- mail = null;
+ public void checkMail(ThreadContext context) {
+ ThreadService.Event myEvent = mail.getAndSet(null);
+
if (myEvent != null) {
switch (myEvent.type) {
case RAISE:
@@ -248,8 +250,12 @@ public void beforeStart() {
/**
* Dispose of the current thread by tidying up connections to other stuff
*/
- public synchronized void dispose() {
- if (!disposed) {
+ public void dispose() {
+ if (disposed) return;
+
+ synchronized (this) {
+ if (disposed) return;
+
disposed = true;
// remove from parent thread group
@@ -265,10 +271,10 @@ public synchronized void dispose() {
// mark thread as DEAD
beDead();
-
- // unregister from runtime's ThreadService
- getRuntime().getThreadService().unregisterThread(this);
}
+
+ // unregister from runtime's ThreadService
+ getRuntime().getThreadService().unregisterThread(this);
}
public static RubyClass createThreadClass(Ruby runtime) {
@@ -808,7 +814,8 @@ public static IRubyObject exit(IRubyObject receiver, Block block) {
synchronized (rubyThread) {
rubyThread.status.set(Status.ABORTING);
- rubyThread.mail = null;
+ // FIXME: This was not checking for non-null before, but maybe it should
+ rubyThread.mail.set(null);
receiver.getRuntime().getThreadService().setCritical(false);
throw new ThreadKill();
}
View
2 core/src/main/java/org/jruby/internal/runtime/ThreadService.java
@@ -342,7 +342,7 @@ public String toString() {
}
}
- public synchronized void deliverEvent(Event event) {
+ public void deliverEvent(Event event) {
// first, check if the sender has unreceived mail
event.sender.checkMail(getCurrentContext());
View
23 spec/regression/GH-1015_thread_kill_and_dispose_can_deadlock_spec.rb
@@ -0,0 +1,23 @@
+require 'rspec'
+
+describe 'A thread dying naturally while being killed' do
+ it 'should not deadlock' do
+ ary = []
+ n = 100
+
+ # This logic is crafted to attempt to maximize likelihood of deadlock.
+ # It could probably be better.
+ n.times do
+ running = false
+ t = Thread.new do
+ running = true
+ end
+ Thread.pass until running
+ t.kill
+ t.join
+ ary << :ok
+ end
+
+ ary.should == [:ok] * n
+ end
+end

0 comments on commit 937f414

Please sign in to comment.
Something went wrong with that request. Please try again.