Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix completeWith exception handling #51734

Merged
merged 1 commit into from
Feb 3, 2020
Merged

Fix completeWith exception handling #51734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions server/src/main/java/org/elasticsearch/action/ActionListener.java
View file
Open in desktop
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to wonder ... maybe (as the eventual goal of our efforts here, don't think it's possible right now), we should just make ActionListener an abstract class and enforce that nothing is thrown in onResponse and onFailure by simply making these lines call some protected innerOnResponse or so.
It seems just enforcing the not throwing in onComplete and map is simply a small part of the overall issue of enforcing the callbacks to handle their own exceptions and we could just dry things up that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree to this, but unfortunately it does not free us from avoiding for instance wrap, since it delegates failures from inner calls to listener.onResponse to onFailure in production (i.e., without assertions).

assert false : ex;
throw ex;
}
}
}
27 changes: 27 additions & 0 deletions server/src/test/java/org/elasticsearch/action/ActionListenerTests.java
View file
Open in desktop
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