Skip to content

Commit

Permalink
Fix completeWith exception handling (#51734)
Browse files Browse the repository at this point in the history
ActionListener.completeWith would catch exceptions from
listener.onResponse and deliver them to lister.onFailure, essentially
double notifying the listener. Instead we now assert that listeners do
not throw when using ActionListener.completeWith.

Relates #50886
  • Loading branch information
henningandersen authored Feb 3, 2020
1 parent 8332ad4 commit 7470dcd
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 2 deletions.
20 changes: 18 additions & 2 deletions server/src/main/java/org/elasticsearch/action/ActionListener.java
Original file line number Diff line number Diff line change
Expand Up @@ -315,12 +315,28 @@ protected void innerOnFailure(Exception e) {
/**
* Completes the given listener with the result from the provided supplier accordingly.
* This method is mainly used to complete a listener with a block of synchronous code.
*
* If the supplier fails, the listener's onFailure handler will be called.
* It is the responsibility of {@code delegate} to handle its own exceptions inside `onResponse` and `onFailure`.
*/
static <Response> void completeWith(ActionListener<Response> listener, CheckedSupplier<Response, ? extends Exception> supplier) {
Response response;
try {
listener.onResponse(supplier.get());
response = supplier.get();
} catch (Exception e) {
listener.onFailure(e);
try {
listener.onFailure(e);
} catch (RuntimeException ex) {
assert false : ex;
throw ex;
}
return;
}
try {
listener.onResponse(response);
} catch (RuntimeException ex) {
assert false : ex;
throw ex;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,33 @@ public void testCompleteWith() {
ActionListener.completeWith(onFailureListener, () -> { throw new IOException("not found"); });
assertThat(onFailureListener.isDone(), equalTo(true));
assertThat(expectThrows(ExecutionException.class, onFailureListener::get).getCause(), instanceOf(IOException.class));

AtomicReference<Exception> exReference = new AtomicReference<>();
ActionListener<String> listener = new ActionListener<>() {
@Override
public void onResponse(String s) {
if (s == null) {
throw new IllegalArgumentException("simulate onResponse exception");
}
}

@Override
public void onFailure(Exception e) {
exReference.set(e);
if (e instanceof IllegalArgumentException) {
throw (IllegalArgumentException) e;
}
}
};

AssertionError assertionError = expectThrows(AssertionError.class, () -> ActionListener.completeWith(listener, () -> null));
assertThat(assertionError.getCause(), instanceOf(IllegalArgumentException.class));
assertNull(exReference.get());

assertionError = expectThrows(AssertionError.class, () -> ActionListener.completeWith(listener,
() -> { throw new IllegalArgumentException(); }));
assertThat(assertionError.getCause(), instanceOf(IllegalArgumentException.class));
assertThat(exReference.get(), instanceOf(IllegalArgumentException.class));
}

/**
Expand Down

0 comments on commit 7470dcd

Please sign in to comment.