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.

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

Transport request handlers should be registered safely #53178

Open
jaymode opened this issue Mar 5, 2020 · 2 comments
Open

Transport request handlers should be registered safely #53178

jaymode opened this issue Mar 5, 2020 · 2 comments
Labels
:Core/Infra/Core Core issues without another label help wanted adoptme >refactoring Team:Core/Infra Meta label for core/infra team

Comments

@jaymode
Copy link
Member

jaymode commented Mar 5, 2020

Similar to #38560 and #51622, the HandledTransportAction registers a non-static inner class with the transport service as the handler for the requests of a given action as seen below.

protected HandledTransportAction(String actionName, boolean canTripCircuitBreaker,
TransportService transportService, ActionFilters actionFilters,
Writeable.Reader<Request> requestReader, String executor) {
super(actionName, actionFilters, transportService.getTaskManager());
transportService.registerRequestHandler(actionName, executor, false, canTripCircuitBreaker, requestReader,
new TransportHandler());
}
class TransportHandler implements TransportRequestHandler<Request> {
@Override
public final void messageReceived(final Request request, final TransportChannel channel, Task task) {
// We already got the task created on the network layer - no need to create it again on the transport layer
execute(task, request, new ChannelActionListener<>(channel, actionName, request));
}
}

In the other issues, a reference to the instance being constructed, this, was explicitly passed to another service. In this case, the reference is implicit since the inner class TransportHandler is not static and has a reference to the outer class. Since the inner class is published and calls methods within the outer class, there is a chance that this could happen before the outer class is fully constructed, which violates the JLS.

For correctness, this code should be changed so that the handler registration no longer occurs within a constructor. However, given the use of guice to construct transport actions and eventual refactoring to remove guice it may be best to consider this as part of that effort.

@jaymode jaymode added :Core/Infra/Core Core issues without another label >refactoring labels Mar 5, 2020
@elasticmachine
Copy link
Collaborator

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

@rjernst rjernst added the Team:Core/Infra Meta label for core/infra team label May 4, 2020
@rjernst rjernst added the needs:triage Requires assignment of a team area label label Dec 3, 2020
@pgomulka pgomulka added help wanted adoptme and removed needs:triage Requires assignment of a team area label labels Dec 14, 2020
@jmbessa
Copy link

jmbessa commented Jun 10, 2021

Hi! I would like to know if this issue is still open. If yes, can I work on it? This would be my first issue

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 help wanted adoptme >refactoring Team:Core/Infra Meta label for core/infra team
Projects
None yet
Development

No branches or pull requests

5 participants