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

Fix TimedRunnable Executing onAfter Twice (#49910) (#49930) 

If we have a nested `AbstractRunnable` inside of `TimedRunnable`
it's executed twice on `run` (once when its own `run` method is invoked and once when
the `onAfter` in the `TimedRunnable` is executed).
Simply removing the `onAfter` override in `TimedRunnable` makes sure that the `onAfter`
is only called once by the `run` on the nested `AbstractRunnable` itself.
Same was done for `onFailure` as it was double-triggering as well on exceptions in the inner `onFailure`.
This commit is contained in:
Armin Braun 2019-12-08 17:36:05 +01:00 committed by GitHub
parent 8ae11e176a
commit f768f8ddab
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 47 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -57,21 +57,10 @@ class TimedRunnable extends AbstractRunnable implements WrappedRunnable {
}
}
@Override
public void onAfter() {
if (original instanceof AbstractRunnable) {
((AbstractRunnable) original).onAfter();
}
}
@Override
public void onFailure(final Exception e) {
this.failedOrRejected = true;
if (original instanceof AbstractRunnable) {
((AbstractRunnable) original).onFailure(e);
} else {
ExceptionsHelper.reThrowIfNotNull(e);
}
ExceptionsHelper.reThrowIfNotNull(e);
}
@Override

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

@ -19,6 +19,7 @@
package org.elasticsearch.common.util.concurrent;
import org.elasticsearch.ExceptionsHelper;
import org.elasticsearch.test.ESTestCase;
import java.util.concurrent.RejectedExecutionException;
@ -33,7 +34,6 @@ public final class TimedRunnableTests extends ESTestCase {
final boolean isForceExecution = randomBoolean();
final AtomicBoolean onAfter = new AtomicBoolean();
final AtomicReference<Exception> onRejection = new AtomicReference<>();
final AtomicReference<Exception> onFailure = new AtomicReference<>();
final AtomicBoolean doRun = new AtomicBoolean();
final AbstractRunnable runnable = new AbstractRunnable() {
@ -54,7 +54,6 @@ public final class TimedRunnableTests extends ESTestCase {
@Override
public void onFailure(final Exception e) {
onFailure.set(e);
}
@Override
@ -67,19 +66,13 @@ public final class TimedRunnableTests extends ESTestCase {
assertThat(timedRunnable.isForceExecution(), equalTo(isForceExecution));
timedRunnable.onAfter();
assertTrue(onAfter.get());
final Exception rejection = new RejectedExecutionException();
timedRunnable.onRejection(rejection);
assertThat(onRejection.get(), equalTo(rejection));
final Exception failure = new Exception();
timedRunnable.onFailure(failure);
assertThat(onFailure.get(), equalTo(failure));
timedRunnable.run();
assertTrue(doRun.get());
assertTrue(onAfter.get());
}
public void testTimedRunnableDelegatesRunInFailureCase() {
@ -144,4 +137,48 @@ public final class TimedRunnableTests extends ESTestCase {
assertSame(exception, thrown);
}
public void testTimedRunnableExecutesNestedOnAfterOnce() {
final AtomicBoolean afterRan = new AtomicBoolean(false);
new TimedRunnable(new AbstractRunnable() {
@Override
public void onFailure(final Exception e) {
}
@Override
protected void doRun() {
}
@Override
public void onAfter() {
if (afterRan.compareAndSet(false, true) == false) {
fail("onAfter should have only been called once");
}
}
}).run();
assertTrue(afterRan.get());
}
public void testNestedOnFailureTriggersOnlyOnce() {
final Exception expectedException = new RuntimeException();
final AtomicBoolean onFailureRan = new AtomicBoolean(false);
RuntimeException thrown = expectThrows(RuntimeException.class, () -> new TimedRunnable(new AbstractRunnable() {
@Override
public void onFailure(Exception e) {
if (onFailureRan.compareAndSet(false, true) == false) {
fail("onFailure should have only been called once");
}
ExceptionsHelper.reThrowIfNotNull(e);
}
@Override
protected void doRun() throws Exception {
throw expectedException;
}
}).run());
assertTrue(onFailureRan.get());
assertSame(thrown, expectedException);
}
}