Bug ID: JDK-6503247 AQS should not handle RuntimeException during acquire specially

Type: Bug
Component: core-libs
Sub-Component: java.util.concurrent
Affected Version: 6

Priority: P3
Status: Closed
Resolution: Fixed
OS: generic
CPU: generic

Submitted: 2006-12-11
Updated: 2011-03-07
Resolved: 2011-03-07

JDK 7
7 b06Fixed

AbstractQueuedSynchronizer and AbstractQueuedLongSynchronizer
handle exceptions during acquire like this

try { ... } 
catch (RuntimeException e) { handle(); throw e; }

This fails to handle errors or other pathological throwables
(e.g. a checked Exception can always be thrown using Thread.stop(Throwable))

More robust and clear is to use the idiom

boolean failed = true;
try { ... ; failed = false; } 
finally { if (failed) handle(); }

EVALUATION Here's a nice addition to the test suite. /* * @test 1.1 06/12/11 * @bug 6503247 * @summary Test resilience to tryAcquire methods that throw * @author Martin Buchholz */ import java.util.*; import java.util.concurrent.*; import java.util.concurrent.locks.*; /** * This uses a variant of the standard Mutex demo, except with a * tryAcquire method that randomly throws various Throwable * subclasses. */ @SuppressWarnings({"deprecation", "serial"}) public class FlakyMutex implements Lock { static class MyError extends Error {} static class MyException extends Exception {} static class MyRuntimeException extends RuntimeException {} static final Random rnd = new Random(); static void maybeThrow() { switch (rnd.nextInt(10)) { case 0: throw new MyError(); case 1: throw new MyRuntimeException(); case 2: Thread.currentThread().stop(new MyException()); break; default: /* Do nothing */ break; } } static void checkThrowable(Throwable t) { check((t instanceof MyError) || (t instanceof MyException) || (t instanceof MyRuntimeException)); } static void realMain(String[] args) throws Throwable { final int nThreads = 3; final CyclicBarrier barrier = new CyclicBarrier(nThreads + 1); final FlakyMutex m = new FlakyMutex(); final ExecutorService es = Executors.newFixedThreadPool(nThreads); for (int i = 0; i < nThreads; i++) { es.submit(new Runnable() { public void run() { try { barrier.await(); for (int i = 0; i < 100; i++) { for (;;) { try { m.lock(); break; } catch (Throwable t) { checkThrowable(t); } } try { check (! m.tryLock()); } catch (Throwable t) { checkThrowable(t); } try { check (! m.tryLock(1, TimeUnit.MICROSECONDS)); } catch (Throwable t) { checkThrowable(t); } m.unlock(); } } catch (Throwable t) { unexpected(t); }}});} barrier.await(); es.shutdown(); check(es.awaitTermination(10, TimeUnit.SECONDS)); } private static class FlakySync extends AbstractQueuedLongSynchronizer { private static final long serialVersionUID = -1L; public boolean isHeldExclusively() { return getState() == 1; } public boolean tryAcquire(long acquires) { maybeThrow(); return compareAndSetState(0, 1); } public boolean tryRelease(long releases) { setState(0); return true; } Condition newCondition() { return new ConditionObject(); } } private final FlakySync sync = new FlakySync(); public void lock() { sync.acquire(1); } public boolean tryLock() { return sync.tryAcquire(1); } public void lockInterruptibly() throws InterruptedException { sync.acquireInterruptibly(1); } public boolean tryLock(long timeout, TimeUnit unit) throws InterruptedException { return sync.tryAcquireNanos(1, unit.toNanos(timeout)); } public void unlock() { sync.release(1); } public Condition newCondition() { return sync.newCondition(); } public boolean isLocked() { return sync.isHeldExclusively(); } public boolean hasQueuedThreads() { return sync.hasQueuedThreads(); } //--------------------- Infrastructure --------------------------- static volatile int passed = 0, failed = 0; static void pass() {passed++;} static void fail() {failed++; Thread.dumpStack();} static void fail(String msg) {System.out.println(msg); fail();} static void unexpected(Throwable t) {failed++; t.printStackTrace();} static void check(boolean cond) {if (cond) pass(); else fail();} static void equal(Object x, Object y) { if (x == null ? y == null : x.equals(y)) pass(); else fail(x + " not equal to " + y);} public static void main(String[] args) throws Throwable { try {realMain(args);} catch (Throwable t) {unexpected(t);} System.out.printf("%nPassed = %d, failed = %d%n%n", passed, failed); if (failed > 0) throw new AssertionError("Some tests failed");} }

11-12-2006

EVALUATION A fine idea. --- /u/martin/ws/dolphin/src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java 2006-08-14 15:35:25.915754000 -0700 +++ /u/martin/ws/RuntimeException/src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java 2006-12-11 13:29:50.454289000 -0800 @@ -504,6 +504,7 @@ * @return {@code true} if interrupted while waiting */ final boolean acquireQueued(final Node node, long arg) { + boolean failed = true; try { boolean interrupted = false; for (;;) { @@ -511,15 +512,16 @@ if (p == head && tryAcquire(arg)) { setHead(node); p.next = null; // help GC + failed = false; return interrupted; } if (shouldParkAfterFailedAcquire(p, node) && parkAndCheckInterrupt()) interrupted = true; } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } } @@ -530,25 +532,24 @@ private void doAcquireInterruptibly(long arg) throws InterruptedException { final Node node = addWaiter(Node.EXCLUSIVE); + boolean failed = true; try { for (;;) { final Node p = node.predecessor(); if (p == head && tryAcquire(arg)) { setHead(node); p.next = null; // help GC + failed = false; return; } if (shouldParkAfterFailedAcquire(p, node) && parkAndCheckInterrupt()) - break; + throw new InterruptedException(); } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } - // Arrive here only if interrupted - cancelAcquire(node); - throw new InterruptedException(); } /** @@ -562,18 +563,18 @@ throws InterruptedException { long lastTime = System.nanoTime(); final Node node = addWaiter(Node.EXCLUSIVE); + boolean failed = true; try { for (;;) { final Node p = node.predecessor(); if (p == head && tryAcquire(arg)) { setHead(node); p.next = null; // help GC + failed = false; return true; } - if (nanosTimeout <= 0) { - cancelAcquire(node); + if (nanosTimeout <= 0) return false; - } if (nanosTimeout > spinForTimeoutThreshold && shouldParkAfterFailedAcquire(p, node)) LockSupport.parkNanos(this, nanosTimeout); @@ -581,15 +582,12 @@ nanosTimeout -= now - lastTime; lastTime = now; if (Thread.interrupted()) - break; + throw new InterruptedException(); } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } - // Arrive here only if interrupted - cancelAcquire(node); - throw new InterruptedException(); } /** @@ -598,6 +596,7 @@ */ private void doAcquireShared(long arg) { final Node node = addWaiter(Node.SHARED); + boolean failed = true; try { boolean interrupted = false; for (;;) { @@ -609,6 +608,7 @@ p.next = null; // help GC if (interrupted) selfInterrupt(); + failed = false; return; } } @@ -616,9 +616,9 @@ parkAndCheckInterrupt()) interrupted = true; } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } } @@ -629,6 +629,7 @@ private void doAcquireSharedInterruptibly(long arg) throws InterruptedException { final Node node = addWaiter(Node.SHARED); + boolean failed = true; try { for (;;) { final Node p = node.predecessor(); @@ -637,20 +638,18 @@ if (r >= 0) { setHeadAndPropagate(node, r); p.next = null; // help GC + failed = false; return; } } if (shouldParkAfterFailedAcquire(p, node) && parkAndCheckInterrupt()) - break; + throw new InterruptedException(); } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } - // Arrive here only if interrupted - cancelAcquire(node); - throw new InterruptedException(); } /** @@ -665,6 +664,7 @@ long lastTime = System.nanoTime(); final Node node = addWaiter(Node.SHARED); + boolean failed = true; try { for (;;) { final Node p = node.predecessor(); @@ -673,13 +673,12 @@ if (r >= 0) { setHeadAndPropagate(node, r); p.next = null; // help GC + failed = false; return true; } } - if (nanosTimeout <= 0) { - cancelAcquire(node); + if (nanosTimeout <= 0) return false; - } if (nanosTimeout > spinForTimeoutThreshold && shouldParkAfterFailedAcquire(p, node)) LockSupport.parkNanos(this, nanosTimeout); @@ -687,15 +686,12 @@ nanosTimeout -= now - lastTime; lastTime = now; if (Thread.interrupted()) - break; + throw new InterruptedException(); } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } - // Arrive here only if interrupted - cancelAcquire(node); - throw new InterruptedException(); } // Main exported methods @@ -1339,17 +1335,19 @@ * @return previous sync state */ final long fullyRelease(Node node) { + boolean failed = true; try { long savedState = getState(); - if (release(savedState)) + if (release(savedState)) { + failed = false; return savedState; - } catch (RuntimeException ex) { - node.waitStatus = Node.CANCELLED; - throw ex; + } else { + throw new IllegalMonitorStateException(); } - // reach here if release fails + } finally { + if (failed) node.waitStatus = Node.CANCELLED; - throw new IllegalMonitorStateException(); + } } // Instrumentation methods for conditions --- /u/martin/ws/dolphin/src/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java 2006-08-14 15:35:26.035005000 -0700 +++ /u/martin/ws/RuntimeException/src/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java 2006-12-11 13:29:51.671199000 -0800 @@ -731,6 +731,7 @@ * @return {@code true} if interrupted while waiting */ final boolean acquireQueued(final Node node, int arg) { + boolean failed = true; try { boolean interrupted = false; for (;;) { @@ -738,15 +739,16 @@ if (p == head && tryAcquire(arg)) { setHead(node); p.next = null; // help GC + failed = false; return interrupted; } if (shouldParkAfterFailedAcquire(p, node) && parkAndCheckInterrupt()) interrupted = true; } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } } @@ -757,25 +759,24 @@ private void doAcquireInterruptibly(int arg) throws InterruptedException { final Node node = addWaiter(Node.EXCLUSIVE); + boolean failed = true; try { for (;;) { final Node p = node.predecessor(); if (p == head && tryAcquire(arg)) { setHead(node); p.next = null; // help GC + failed = false; return; } if (shouldParkAfterFailedAcquire(p, node) && parkAndCheckInterrupt()) - break; + throw new InterruptedException(); } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } - // Arrive here only if interrupted - cancelAcquire(node); - throw new InterruptedException(); } /** @@ -789,18 +790,18 @@ throws InterruptedException { long lastTime = System.nanoTime(); final Node node = addWaiter(Node.EXCLUSIVE); + boolean failed = true; try { for (;;) { final Node p = node.predecessor(); if (p == head && tryAcquire(arg)) { setHead(node); p.next = null; // help GC + failed = false; return true; } - if (nanosTimeout <= 0) { - cancelAcquire(node); + if (nanosTimeout <= 0) return false; - } if (nanosTimeout > spinForTimeoutThreshold && shouldParkAfterFailedAcquire(p, node)) LockSupport.parkNanos(this, nanosTimeout); @@ -808,15 +809,12 @@ nanosTimeout -= now - lastTime; lastTime = now; if (Thread.interrupted()) - break; + throw new InterruptedException(); } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } - // Arrive here only if interrupted - cancelAcquire(node); - throw new InterruptedException(); } /** @@ -825,6 +823,7 @@ */ private void doAcquireShared(int arg) { final Node node = addWaiter(Node.SHARED); + boolean failed = true; try { boolean interrupted = false; for (;;) { @@ -836,6 +835,7 @@ p.next = null; // help GC if (interrupted) selfInterrupt(); + failed = false; return; } } @@ -843,9 +843,9 @@ parkAndCheckInterrupt()) interrupted = true; } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } } @@ -856,6 +856,7 @@ private void doAcquireSharedInterruptibly(int arg) throws InterruptedException { final Node node = addWaiter(Node.SHARED); + boolean failed = true; try { for (;;) { final Node p = node.predecessor(); @@ -864,20 +865,18 @@ if (r >= 0) { setHeadAndPropagate(node, r); p.next = null; // help GC + failed = false; return; } } if (shouldParkAfterFailedAcquire(p, node) && parkAndCheckInterrupt()) - break; + throw new InterruptedException(); } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } - // Arrive here only if interrupted - cancelAcquire(node); - throw new InterruptedException(); } /** @@ -892,6 +891,7 @@ long lastTime = System.nanoTime(); final Node node = addWaiter(Node.SHARED); + boolean failed = true; try { for (;;) { final Node p = node.predecessor(); @@ -900,13 +900,12 @@ if (r >= 0) { setHeadAndPropagate(node, r); p.next = null; // help GC + failed = false; return true; } } - if (nanosTimeout <= 0) { - cancelAcquire(node); + if (nanosTimeout <= 0) return false; - } if (nanosTimeout > spinForTimeoutThreshold && shouldParkAfterFailedAcquire(p, node)) LockSupport.parkNanos(this, nanosTimeout); @@ -914,15 +913,12 @@ nanosTimeout -= now - lastTime; lastTime = now; if (Thread.interrupted()) - break; + throw new InterruptedException(); } - } catch (RuntimeException ex) { + } finally { + if (failed) cancelAcquire(node); - throw ex; } - // Arrive here only if interrupted - cancelAcquire(node); - throw new InterruptedException(); } // Main exported methods @@ -1566,17 +1562,19 @@ * @return previous sync state */ final int fullyRelease(Node node) { + boolean failed = true; try { int savedState = getState(); - if (release(savedState)) + if (release(savedState)) { + failed = false; return savedState; - } catch (RuntimeException ex) { - node.waitStatus = Node.CANCELLED; - throw ex; + } else { + throw new IllegalMonitorStateException(); } - // reach here if release fails + } finally { + if (failed) node.waitStatus = Node.CANCELLED; - throw new IllegalMonitorStateException(); + } } // Instrumentation methods for conditions

11-12-2006