honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Do not swallow exceptions in TimedRunnable (#39856) 

Executors of type fixed_auto_queue_size (i.e. search / search_throttled) wrap runnables into
TimedRunnable, which is an AbstractRunnable. This is dangerous as it might silently swallow
exceptions, and possibly miss calling a response listener. While this has not triggered any failures in
the tests I have run so far, it might help uncover future problems.

Follow-up to #36137
This commit is contained in:
Yannick Welsch 2019-03-11 18:59:31 +01:00
parent 292eb8b001
commit 4f941c6963
3 changed files with 39 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
qa/evil-tests/src/test/java/org/elasticsearch/threadpool/EvilThreadPoolTests.java
View File

@ -163,20 +163,12 @@ public class EvilThreadPoolTests extends ESTestCase {
public void testExecutionExceptionOnDefaultThreadPoolTypes() throws InterruptedException {
for (String executor : ThreadPool.THREAD_POOL_TYPES.keySet()) {
final boolean expectExceptionOnExecute =
// fixed_auto_queue_size wraps stuff into TimedRunnable, which is an AbstractRunnable
// TODO: this is dangerous as it will silently swallow exceptions, and possibly miss calling a response listener
ThreadPool.THREAD_POOL_TYPES.get(executor) != ThreadPool.ThreadPoolType.FIXED_AUTO_QUEUE_SIZE;
checkExecutionException(getExecuteRunner(threadPool.executor(executor)), expectExceptionOnExecute);
checkExecutionException(getExecuteRunner(threadPool.executor(executor)), true);
// here, it's ok for the exception not to bubble up. Accessing the future will yield the exception
checkExecutionException(getSubmitRunner(threadPool.executor(executor)), false);
final boolean expectExceptionOnSchedule =
// fixed_auto_queue_size wraps stuff into TimedRunnable, which is an AbstractRunnable
// TODO: this is dangerous as it will silently swallow exceptions, and possibly miss calling a response listener
ThreadPool.THREAD_POOL_TYPES.get(executor) != ThreadPool.ThreadPoolType.FIXED_AUTO_QUEUE_SIZE;
checkExecutionException(getScheduleRunner(executor), expectExceptionOnSchedule);
checkExecutionException(getScheduleRunner(executor), true);
}
}
@ -213,8 +205,7 @@ public class EvilThreadPoolTests extends ESTestCase {
1, 1, 1, TimeValue.timeValueSeconds(10), EsExecutors.daemonThreadFactory("test"), threadPool.getThreadContext());
try {
// fixed_auto_queue_size wraps stuff into TimedRunnable, which is an AbstractRunnable
// TODO: this is dangerous as it will silently swallow exceptions, and possibly miss calling a response listener
checkExecutionException(getExecuteRunner(autoQueueFixedExecutor), false);
checkExecutionException(getExecuteRunner(autoQueueFixedExecutor), true);
checkExecutionException(getSubmitRunner(autoQueueFixedExecutor), false);
} finally {
ThreadPool.terminate(autoQueueFixedExecutor, 10, TimeUnit.SECONDS);

6
server/src/main/java/org/elasticsearch/common/util/concurrent/TimedRunnable.java
View File

@ -19,6 +19,8 @@
package org.elasticsearch.common.util.concurrent;
import org.elasticsearch.ExceptionsHelper;
/**
* A class used to wrap a {@code Runnable} that allows capturing the time of the task since creation
* through execution as well as only execution time.
@ -48,6 +50,8 @@ class TimedRunnable extends AbstractRunnable implements WrappedRunnable {
public void onRejection(final Exception e) {
if (original instanceof AbstractRunnable) {
((AbstractRunnable) original).onRejection(e);
} else {
ExceptionsHelper.reThrowIfNotNull(e);
}
}
@ -62,6 +66,8 @@ class TimedRunnable extends AbstractRunnable implements WrappedRunnable {
public void onFailure(final Exception e) {
if (original instanceof AbstractRunnable) {
((AbstractRunnable) original).onFailure(e);
} else {
ExceptionsHelper.reThrowIfNotNull(e);
}
}

30
server/src/test/java/org/elasticsearch/common/util/concurrent/TimedRunnableTests.java
View File

@ -114,4 +114,34 @@ public final class TimedRunnableTests extends ESTestCase {
assertTrue(onAfter.get());
}
public void testTimedRunnableRethrowsExceptionWhenNotAbstractRunnable() {
final AtomicBoolean hasRun = new AtomicBoolean();
final RuntimeException exception = new RuntimeException();
final Runnable runnable = () -> {
hasRun.set(true);
throw exception;
};
final TimedRunnable timedRunnable = new TimedRunnable(runnable);
final RuntimeException thrown = expectThrows(RuntimeException.class, () -> timedRunnable.run());
assertTrue(hasRun.get());
assertSame(exception, thrown);
}
public void testTimedRunnableRethrowsRejectionWhenNotAbstractRunnable() {
final AtomicBoolean hasRun = new AtomicBoolean();
final RuntimeException exception = new RuntimeException();
final Runnable runnable = () -> {
hasRun.set(true);
throw new AssertionError("should not run");
};
final TimedRunnable timedRunnable = new TimedRunnable(runnable);
final RuntimeException thrown = expectThrows(RuntimeException.class, () -> timedRunnable.onRejection(exception));
assertFalse(hasRun.get());
assertSame(exception, thrown);
}
}