Index: test/test_thread.rb =================================================================== --- test/test_thread.rb (revision 3899) +++ test/test_thread.rb (working copy) @@ -9,6 +9,91 @@ assert_equal(1, $toto) assert_equal(false, thread.status) end + + # This tests for a found bug in Thread#critical (JRUBY-1157 at jira.codehaus.org). + # + # Background: + # + # - In MRI, setting 'Thread.critical = true' means no other threads can possibly + # be scheduled, period. (This is because MRI uses 'green threads', meaning it + # implements its own thread scheduler, and the OS knows nothing about the + # threads in the process; as such, it can guarantee this easily.) Thus it is + # simply not possible for another thread to set Thread.critical to anything, + # period, until the first thread sets 'Thread.critical = false'. + # + # - In JRuby, Ruby threads are mapped directly onto Java threads. Because of this, + # JRuby cannot actually guarantee that no other threads are scheduled once you + # set 'Thread.critical = true'; all it can do is block other threads as soon as + # they call one of a certain set of library functions that explicitly check the + # value of 'Thread.critical'. + # + # The bug involves two threads: + # + # 1. Thread A calls 'Thread.critical = true'. + # 2. Thread B is then selected to run by the OS's scheduler. In MRI, this is not + # possible, but in JRuby, it is. + # 3. Thread B calls 'Thread.critical = true'. This correctly does the right thing, + # which is to block until Thread A sets 'Thread.critical = false'. However, + # here is where the bug occurred: this would grab a monitor (an ordinary Java + # monitor) that was required to do anything at all with the 'critical lock', + # and hold it while it was blocked on a *different* lock waiting for thread A + # to set 'Thread.critical = false'. (Because it is blocked on a different lock, + # Java's synchronization primitives do not automatically release the monitor + # this thread has acquited.) + # 4. Thread A calls 'Thread.critical = false'. However, in order to change the + # value, it needs to acquire the monitor that thread A holds. + # 5. Deadlock. + # + # The fix was to rework the synchronization so that the same monitor is used for + # both locking the value of Thread.critical and to block until it's set to 'false' + # when required, above. This test case demonstrates that the bug is fixed, and + # makes sure it never recurs. + # + # The bug was nasty because lots of synchronization objects (e.g., Mutex) use + # Thread.critical in their implementation, and it ends up just being a matter of + # time until the entire system deadlocks. (In particular, even just calls to + # Logger from different threads will do this, as it uses a Mutex, which, in + # turn, uses Thread.critical.) + # + # Unfortunately, this test case simply *hangs* when the bug is present, as that's + # the consequence of the bug (it's a deadlock). Further, we can't do anything + # about it, as using another thread to try to time out the test just exacerbates + # the problem and deadlocks itself. Alas, that's all we can do, but at least a + # hung test is likely to get noticed when compared to no test at all. :-) + # + def test_critical + puts "If the tests hang at this point (for more than a minute or so), it " + puts "means a test has failed; see #{__FILE__}:#{__LINE__} for more information." + + # This thread pauses a moment, then sets Thread.critical to true first. + # It then waits long enough for the second thread to end up blocked in + # its own call to 'Thread.critical = true', then tries to set + # Thread.critical back to false. If the bug is present, this thread gets + # deadlocked when it tries to set Thread.critical back to false; if it's + # not present, it passes through, unblocks the second thread, and all is + # well. + thread1 = Thread.new { + sleep 3 + Thread.critical = true + sleep 9 + Thread.critical = false + } + + # This thread pauses long enough that Thread.critical will already be + # true by the time it tries to set it to true again, making it block; + # once the first thread sets it to false, it will then be able to + # proceed. + thread2 = Thread.new { + sleep 6 + Thread.critical = true + sleep 2 + Thread.critical = false + } + + # Join the threads to make sure all is well -- then we're done. + thread1.join + thread2.join + end def test_local_variables v = nil Index: src/org/jruby/internal/runtime/ThreadService.java =================================================================== --- src/org/jruby/internal/runtime/ThreadService.java (revision 3899) +++ src/org/jruby/internal/runtime/ThreadService.java (working copy) @@ -52,7 +52,8 @@ private Set rubyThreadList; private Thread mainThread; - private ReentrantLock criticalLock = new ReentrantLock(); + private RubyThread currentCriticalLockHolder = null; + private final Object criticalLockObject = new Object(); public ThreadService(Ruby runtime) { this.runtime = runtime; @@ -158,31 +159,49 @@ return rubyThread; } - public synchronized void setCritical(boolean critical) { - if (criticalLock.isHeldByCurrentThread()) { - if (critical) { - // do nothing - } else { - criticalLock.unlock(); - } - } else { - if (critical) { - criticalLock.lock(); - } else { - // do nothing - } - } + private void waitQuietlyForCriticalLockObject() { + try { + criticalLockObject.wait(); + } catch (InterruptedException ie) { + // intentionally ignored + } } - - public synchronized boolean getCritical() { - return criticalLock.isHeldByCurrentThread(); + + private RubyThread currentRubyThread() { + return getRubyThreadFromThread(Thread.currentThread()); } - + + public void setCritical(boolean critical) { + synchronized (criticalLockObject) { + RubyThread currentRubyThread = currentRubyThread(); + + if (critical) { + waitForCritical(); + currentCriticalLockHolder = currentRubyThread; + criticalLockObject.notifyAll(); + } else { + if (currentCriticalLockHolder == currentRubyThread) { + currentCriticalLockHolder = null; + criticalLockObject.notifyAll(); + } + } + } + } + + public boolean getCritical() { + synchronized (criticalLockObject) { + return currentCriticalLockHolder != null; + } + } + public void waitForCritical() { - if (criticalLock.isLocked()) { - criticalLock.lock(); - criticalLock.unlock(); - } + synchronized (criticalLockObject) { + RubyThread thisThread = currentRubyThread(); + while (currentCriticalLockHolder != null + && currentCriticalLockHolder != thisThread) { + waitQuietlyForCriticalLockObject(); + } + } } }