From e2c0a2ad1e3db5463881e3ffea285f4e33a7b216 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Wed, 10 Sep 2014 11:55:02 +0200 Subject: [PATCH] BUG-1786: fixup DeadlockDetectingListeningExecutorService 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 --- ...lockDetectingListeningExecutorService.java | 115 ++++++++++++++---- .../util/concurrent/SettableBoolean.java | 39 ++++++ .../SettableBooleanThreadLocal.java | 23 ++++ 3 files changed, 156 insertions(+), 21 deletions(-) create mode 100644 common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/SettableBoolean.java create mode 100644 common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/SettableBooleanThreadLocal.java diff --git a/common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/DeadlockDetectingListeningExecutorService.java b/common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/DeadlockDetectingListeningExecutorService.java index 011872d6b1..958f2ee511 100644 --- a/common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/DeadlockDetectingListeningExecutorService.java +++ b/common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/DeadlockDetectingListeningExecutorService.java @@ -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 get 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 deadlockDetector = new ThreadLocal<>(); - private final Function 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 deadlockExceptionFunction; + + // Compatibility wrapper, needs to be removed once the deprecated constructors are gone. + private static final class CompatExceptionSupplier implements Supplier { + private final Function function; + + private CompatExceptionSupplier(final Function 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 deadlockExceptionFunction ) { + @Deprecated + public DeadlockDetectingListeningExecutorService(final ExecutorService delegate, + final Function 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 deadlockExceptionFunction, - @Nullable Executor listenableFutureExecutor ) { + @Deprecated + public DeadlockDetectingListeningExecutorService(final ExecutorService delegate, + final Function 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 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 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 ListenableFuture submit( Callable task ){ + public ListenableFuture submit(final Callable 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 ListenableFuture submit( Runnable task, T result ){ + public ListenableFuture 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() { @Override public T call() throws Exception { - deadlockDetector.set(Boolean.TRUE); + final SettableBoolean b = primeDetector(); try { return delagate.call(); } finally { - deadlockDetector.remove(); + b.reset(); } } }; } - private ListenableFuture wrapListenableFuture(final ListenableFuture delegate ) { + private ListenableFuture wrapListenableFuture(final ListenableFuture 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 index 0000000000..0d584af1fe --- /dev/null +++ b/common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/SettableBoolean.java @@ -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 index 0000000000..8826f99ddb --- /dev/null +++ b/common/util/src/main/java/org/opendaylight/yangtools/util/concurrent/SettableBooleanThreadLocal.java @@ -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 { + @Override + protected SettableBoolean initialValue() { + return new SettableBoolean(); + } + + @Override + public void set(final SettableBoolean value) { + throw new UnsupportedOperationException("Resetting the value is not supported"); + } +} -- 2.36.6