elastic/elasticsearch

Transport request handlers should be registered safely

Open

#53,178 opened on Mar 5, 2020

View on GitHub
 (2 comments) (0 reactions) (0 assignees)Java (76,700 stars) (25,882 forks)batch import
:Core/Infra/Core>refactoringTeam:Core/Infrahelp wanted

Description

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.

https://github.com/elastic/elasticsearch/blob/898012028e5cd9c6d6058d2063d772fabdea305c/server/src/main/java/org/elasticsearch/action/support/HandledTransportAction.java#L52-L66

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.

Contributor guide

Tech stack
java
Domain
backend
Issue type
bug
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
4
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
3-5 days
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
stale
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
clear
Prerequisites
Java concurrencyElasticsearch transport layer
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
20
Research direction
Investigate the HandledTransportAction class at the linked file (server/src/main/java/org/elasticsearch/action/support/HandledTransportAction.java). Review the similar issues #38560 and #51622 to understand the pattern and how they fixed the unsafe constructor reference. Since maintainers note this may be part of guice removal, propose a solution that separates handler registration from construction, possibly using a factory or post construct method.