Skip to content

Conversation

@DaveCTurner
Copy link
Contributor

Today if the exceptional completion of a ThreadedActionListener is rejected from its executor then the listener is completed with the original exception on the completing thread, with the exception representing the rejection added to its suppressed exceptions list. In practice this is a little trappy, we may want to handle the rejection differently but won't always remember to check the suppressed exceptions list for a rejection.

This commit reverses the order of exceptions passed to the delegate listener in this case: the rejection exception is at the top level, with the original exception added to its suppressed exceptions list.

Today if the exceptional completion of a `ThreadedActionListener` is
rejected from its executor then the listener is completed with the
original exception on the completing thread, with the exception
representing the rejection added to its suppressed exceptions list. In
practice this is a little trappy, we may want to handle the rejection
differently but won't always remember to check the suppressed exceptions
list for a rejection.

This commit reverses the order of exceptions passed to the delegate
listener in this case: the rejection exception is at the top level, with
the original exception added to its suppressed exceptions list.
@DaveCTurner DaveCTurner added >non-issue :Core/Infra/Core Core issues without another label v8.8.0 labels Mar 7, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 7, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@DaveCTurner
Copy link
Contributor Author

DaveCTurner commented Mar 7, 2023

>non-issue because all the production usages of ThreadedActionListener today do not fork to a threadpool which rejects anything anyway. The motivation for this change is to align the rejection behaviour between ThreadedActionListener and ListenableFuture so that we can simplify ListenableFuture a little.

Copy link
Member

@thecoop thecoop left a comment

Choose a reason for hiding this comment

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

Looks sensible

@DaveCTurner DaveCTurner merged commit ba671e4 into elastic:main Mar 7, 2023
@DaveCTurner DaveCTurner deleted the 2023-03-07-ThreadedActionListener-rejection-suppression branch March 7, 2023 13:12
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Mar 7, 2023
Following elastic#94363 we can use `ThreadedActionListener` to represent the
forking that happens within `ListenableFuture`, rather than tracking the
executor and listener as separate components of a Tuple.
DaveCTurner added a commit that referenced this pull request Mar 7, 2023
Following #94363 we can use `ThreadedActionListener` to represent the
forking that happens within `ListenableFuture`, rather than tracking the
executor and listener as separate components of a Tuple.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants