Fix completeWith exception handling #51734
Conversation
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 elastic#50886
henningandersen
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (:Core/Infra/Core) |
|
Pinging @elastic/es-distributed (:Distributed/Distributed) |
| } | ||
| try { | ||
| listener.onResponse(response); | ||
| } catch (RuntimeException ex) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
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
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