Remove dangerous default executor from HandledTransportAction #100162
Conversation
Today subclasses of `HandledTransportAction` can specify the executor on which they run, but the executor is optional and if omitted will use `DIRECT_EXECUTOR_SERVICE`, which means the action runs on a transport thread. This is a dangerous default behaviour because it makes it easy to add new transport actions which implicitly run on a network thread, which is very hard to pick up in reviews. This commit makes the executor explicit in all callers, and marks the dangerous methods for removal.
DaveCTurner
added
:Distributed/Network
|
@elasticmachine please run elasticsearch-ci/part-2 |
|
Pinging @elastic/es-distributed (Team:Distributed) |
No need for callers to specify `canTripCircuitBreaker`, the default for this is safe.
|
NB this was almost completely mechanical, just automatically inlining the bad methods within |
| import org.elasticsearch.transport.TransportService; | ||
|
|
||
| import java.util.concurrent.Executor; | ||
|
|
||
| /** | ||
| * A TransportAction that self registers a handler into the transport service | ||
| * A {@link TransportAction} which registers a handler for itself with the transport service. |
There was a problem hiding this comment.
[opt] not really sure what 'self registers' or now 'for itself' add in terms of information. Could perhaps remove entirely -- or elaborate, if there is some meaning I'm missing.
There was a problem hiding this comment.
Hmm yeah in a sense what other action could we really be registering here? But we do register subsidiary handlers for actions with names like actionName + "[x]" elsewhere. I changed it to make it clearer that we're not mangling the name at all with the registration.
There was a problem hiding this comment.
Gotcha, that's a better lead on what it's doing, thank you.
I don't think I myself have an understanding of what registering action names means in the bigger picture, but that's not something to be documented in this patch.
There was a problem hiding this comment.
It's not very well documented today; I opened #100229
server/src/main/java/org/elasticsearch/action/support/HandledTransportAction.java
Show resolved
Hide resolved
DaveCTurner
added
the
auto-merge
DaveCTurner
deleted the
2023/10/02/HandledTransportAction-default-executor
branch
These things are no longer used. Relates elastic#100162
These things are no longer used. Relates #100162
DaveCTurner
restored the
2023/10/02/HandledTransportAction-default-executor
branch
Today subclasses of
HandledTransportActioncan specify the executor onwhich they run, but the executor is optional and if omitted will use
DIRECT_EXECUTOR_SERVICE, which means the action runs on a transportthread. This is a dangerous default behaviour because it makes it easy
to add new transport actions which implicitly run on a network thread,
which is very hard to pick up in reviews.
This commit makes the executor explicit in all callers, and marks the
dangerous methods for removal.