BUG-1786: fixup DeadlockDetectingListeningExecutorService 84/10984/3
authorRobert Varga <rovarga@cisco.com>
Wed, 10 Sep 2014 09:55:02 +0000 (11:55 +0200)
committerRobert Varga <rovarga@cisco.com>
Wed, 10 Sep 2014 15:04:29 +0000 (17:04 +0200)
This patch deprecates the use of a Function where a Supplier should be
used.

Furthermore it shifts the balance of the ThreadLocal interaction by
retaining switching a Boolean to a SettableBoolean and flipping the
field contained within it rather than setting/removing the thread local
value itself. This can leave a single object attached to a thread --
which can be removed by a call to cleanStateForCurrentThread(), but
improves the performance of the fast path by utilizing the
ThreadLocal.get() fastpath when used with threadpools.

Change-Id: I2d8897138f8719187425e3aa8121bdc0f1ff39b2
Signed-off-by: Robert Varga <rovarga@cisco.com>
common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/DeadlockDetectingListeningExecutorService.java
common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/SettableBoolean.java [new file with mode: 0644]
common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/SettableBooleanThreadLocal.java [new file with mode: 0644]

index 011872d6b138d9edbaf28869524121bd859f0b43..958f2ee5118b265ccdde95cd8ed9891fdde373bb 100644 (file)
@@ -8,11 +8,12 @@
 
 package org.opendaylight.yangtools.util.concurrent;
 
-import static com.google.common.base.Preconditions.checkNotNull;
-
 import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
 import com.google.common.util.concurrent.ForwardingListenableFuture;
 import com.google.common.util.concurrent.ListenableFuture;
+
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
@@ -20,6 +21,7 @@ import java.util.concurrent.ExecutorService;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 
+import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
 /**
@@ -42,11 +44,35 @@ import javax.annotation.Nullable;
  * from this class override the <code>get</code> methods to check if the ThreadLocal is set. If it is,
  * an ExecutionException is thrown with a custom cause.
  *
+ * Note that the ThreadLocal is not removed automatically, so some state may be left hanging off of
+ * threads which have encountered this class. If you need to clean that state up, use
+ * {@link #cleanStateForCurrentThread()}.
+ *
  * @author Thomas Pantelis
+ * @author Robert Varga
  */
 public class DeadlockDetectingListeningExecutorService extends AsyncNotifyingListeningExecutorService {
-    private final ThreadLocal<Boolean> deadlockDetector = new ThreadLocal<>();
-    private final Function<Void, Exception> deadlockExceptionFunction;
+    /*
+     * We cannot use a static field simply because our API contract allows nesting, which means some
+     * tasks may be submitted to underlay and some to overlay service -- and the two cases need to
+     * be discerned reliably.
+     */
+    private final SettableBooleanThreadLocal deadlockDetector = new SettableBooleanThreadLocal();
+    private final Supplier<Exception> deadlockExceptionFunction;
+
+    // Compatibility wrapper, needs to be removed once the deprecated constructors are gone.
+    private static final class CompatExceptionSupplier implements Supplier<Exception> {
+        private final Function<Void, Exception> function;
+
+        private CompatExceptionSupplier(final Function<Void, Exception> function) {
+            this.function = Preconditions.checkNotNull(function);
+        }
+
+        @Override
+        public Exception get() {
+            return function.apply(null);
+        }
+    }
 
     /**
      * Constructor.
@@ -54,9 +80,11 @@ public class DeadlockDetectingListeningExecutorService extends AsyncNotifyingLis
      * @param delegate the backing ExecutorService.
      * @param deadlockExceptionFunction Function that returns an Exception instance to set as the
      *             cause of the ExecutionException when a deadlock is detected.
+     * @deprecated Use {@link #DeadlockDetectingListeningExecutorService(ExecutorService, Supplier)} instead.
      */
-    public DeadlockDetectingListeningExecutorService( ExecutorService delegate,
-                                          Function<Void,Exception> deadlockExceptionFunction ) {
+    @Deprecated
+    public DeadlockDetectingListeningExecutorService(final ExecutorService delegate,
+            final Function<Void, Exception> deadlockExceptionFunction) {
         this(delegate, deadlockExceptionFunction, null);
     }
 
@@ -68,43 +96,88 @@ public class DeadlockDetectingListeningExecutorService extends AsyncNotifyingLis
      *             cause of the ExecutionException when a deadlock is detected.
      * @param listenableFutureExecutor the executor used to run listener callbacks asynchronously.
      *             If null, no executor is used.
+     * @deprecated Use {@link #DeadlockDetectingListeningExecutorService(ExecutorService, Supplier, Executor)} instead.
      */
-    public DeadlockDetectingListeningExecutorService( ExecutorService delegate,
-                                          Function<Void,Exception> deadlockExceptionFunction,
-                                          @Nullable Executor listenableFutureExecutor ) {
+    @Deprecated
+    public DeadlockDetectingListeningExecutorService(final ExecutorService delegate,
+            final Function<Void, Exception> deadlockExceptionFunction,
+            @Nullable final Executor listenableFutureExecutor) {
         super(delegate, listenableFutureExecutor);
-        this.deadlockExceptionFunction = checkNotNull(deadlockExceptionFunction);
+        this.deadlockExceptionFunction = new CompatExceptionSupplier(deadlockExceptionFunction);
+    }
+
+    /**
+     * Constructor.
+     *
+     * @param delegate the backing ExecutorService.
+     * @param deadlockExceptionSupplier Supplier that returns an Exception instance to set as the
+     *             cause of the ExecutionException when a deadlock is detected.
+     */
+    public DeadlockDetectingListeningExecutorService(final ExecutorService delegate,
+            @Nonnull final Supplier<Exception> deadlockExceptionSupplier) {
+        this(delegate, deadlockExceptionSupplier, null);
+    }
+
+    /**
+     * Constructor.
+     *
+     * @param delegate the backing ExecutorService.
+     * @param deadlockExceptionSupplier Supplier that returns an Exception instance to set as the
+     *             cause of the ExecutionException when a deadlock is detected.
+     * @param listenableFutureExecutor the executor used to run listener callbacks asynchronously.
+     *             If null, no executor is used.
+     */
+    public DeadlockDetectingListeningExecutorService(final ExecutorService delegate,
+            @Nonnull final Supplier<Exception> deadlockExceptionSupplier,
+            @Nullable final Executor listenableFutureExecutor ) {
+        super(delegate, listenableFutureExecutor);
+        this.deadlockExceptionFunction = Preconditions.checkNotNull(deadlockExceptionSupplier);
     }
 
     @Override
-    public void execute( Runnable command ){
+    public void execute(final Runnable command) {
         getDelegate().execute(wrapRunnable(command));
     }
 
     @Override
-    public <T> ListenableFuture<T> submit( Callable<T> task ){
+    public <T> ListenableFuture<T> submit(final Callable<T> task) {
         return wrapListenableFuture(super.submit(wrapCallable(task)));
     }
 
     @Override
-    public ListenableFuture<?> submit( Runnable task ){
+    public ListenableFuture<?> submit(final Runnable task) {
         return wrapListenableFuture(super.submit(wrapRunnable(task)));
     }
 
     @Override
-    public <T> ListenableFuture<T> submit( Runnable task, T result ){
+    public <T> ListenableFuture<T> submit(final Runnable task, final T result) {
         return wrapListenableFuture(super.submit(wrapRunnable(task), result));
     }
 
+    /**
+     * Remove the state this instance may have attached to the calling thread. If no state
+     * was attached this method does nothing.
+     */
+    public void cleanStateForCurrentThread() {
+        deadlockDetector.remove();
+    }
+
+    private SettableBoolean primeDetector() {
+        final SettableBoolean b = deadlockDetector.get();
+        Preconditions.checkState(!b.isSet(), "Detector for {} has already been primed", this);
+        b.set();
+        return b;
+    }
+
     private Runnable wrapRunnable(final Runnable task) {
         return new Runnable() {
             @Override
             public void run() {
-                deadlockDetector.set(Boolean.TRUE);
+                final SettableBoolean b = primeDetector();
                 try {
                     task.run();
                 } finally {
-                    deadlockDetector.remove();
+                    b.reset();
                 }
             }
         };
@@ -114,17 +187,17 @@ public class DeadlockDetectingListeningExecutorService extends AsyncNotifyingLis
         return new Callable<T>() {
             @Override
             public T call() throws Exception {
-                deadlockDetector.set(Boolean.TRUE);
+                final SettableBoolean b = primeDetector();
                 try {
                     return delagate.call();
                 } finally {
-                    deadlockDetector.remove();
+                    b.reset();
                 }
             }
         };
     }
 
-    private <T> ListenableFuture<T> wrapListenableFuture(final ListenableFuture<T> delegate ) {
+    private <T> ListenableFuture<T> wrapListenableFuture(final ListenableFuture<T> delegate) {
         /*
          * This creates a forwarding Future that overrides calls to get(...) to check, via the
          * ThreadLocal, if the caller is doing a blocking call on a thread from this executor. If
@@ -148,9 +221,9 @@ public class DeadlockDetectingListeningExecutorService extends AsyncNotifyingLis
             }
 
             void checkDeadLockDetectorTL() throws ExecutionException {
-                if (deadlockDetector.get() != null) {
+                if (deadlockDetector.get().isSet()) {
                     throw new ExecutionException("A potential deadlock was detected.",
-                            deadlockExceptionFunction.apply(null));
+                            deadlockExceptionFunction.get());
                 }
             }
         };
diff --git a/common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/SettableBoolean.java b/common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/SettableBoolean.java
new file mode 100644 (file)
index 0000000..0d584af
--- /dev/null
@@ -0,0 +1,39 @@
+/*
+ * Copyright (c) 2014 Cisco Systems, Inc. and others.  All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.yangtools.util.concurrent;
+
+/**
+ * Simple container encapsulating a boolean flag, which can be toggled. It starts
+ * off in the reset state.
+ */
+final class SettableBoolean {
+    private boolean value = false;
+
+    /**
+     * Set the flag to its initial (false) state.
+     */
+    public void reset() {
+        value = false;
+    }
+
+    /**
+     * Set the flag.
+     */
+    public void set() {
+        value = true;
+    }
+
+    /**
+     * Query the flag.
+     *
+     * @return True if the flag has been set since instantiation or last {@link #reset()}.
+     */
+    public boolean isSet() {
+        return value;
+    }
+}
diff --git a/common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/SettableBooleanThreadLocal.java b/common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/SettableBooleanThreadLocal.java
new file mode 100644 (file)
index 0000000..8826f99
--- /dev/null
@@ -0,0 +1,23 @@
+/*
+ * Copyright (c) 2014 Cisco Systems, Inc. and others.  All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.yangtools.util.concurrent;
+
+/**
+ * A reusable {@link ThreadLocal} which returns a {@link SettableBoolean}.
+ */
+final class SettableBooleanThreadLocal extends ThreadLocal<SettableBoolean> {
+    @Override
+    protected SettableBoolean initialValue() {
+        return new SettableBoolean();
+    }
+
+    @Override
+    public void set(final SettableBoolean value) {
+        throw new UnsupportedOperationException("Resetting the value is not supported");
+    }
+}