HDDS-14793. Fix race condition in XceiverClientGrpc#connectToDatanode causing intermittent NPEs. by ptlrs · Pull Request #9997 · apache/ozone

ptlrs · 2026-03-29T07:04:05Z

What changes were proposed in this pull request?

The XceiverClientGrpc#connectToDatanode intermittently fails with an NPE.
The problem is that for a given datanode, there is a race condition between creating a channel and creating a stub.

When a new channel is created for a DN, it is put into the channels map. However, presence of a channel in the map does not imply that the corresponding stub for the same DN also exists in the asyncStubs map.

If the stub is accessed after creating a channel but before the creation of stub, we can get an NPE.

This PR fixes the problem by:

maintaining only one dnChannelInfoMap for both the channels and stubs instead of two independent maps
creating a ChannelInfo class to group the channel and stub

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14793

How was this patch tested?

CI: https://github.com/ptlrs/ozone/actions/runs/23703558972
Flaky test runner: https://github.com/ptlrs/ozone/actions/runs/23823575690

…rpc#sendCommandAsync`.

…e` causing intermittent NPEs.

adoroszlai

Thanks @ptlrs for working on this.

adoroszlai · 2026-03-29T07:47:48Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

+  /**
+   * Checks if the client has a live connection channel to the specified Datanode.
+   *
+   * @return True if the connection is alive, false otherwise.
+   */
+  @VisibleForTesting
+  public boolean isConnected(DatanodeDetails details) {


nit: please don't move isConnected unnecessarily, it inflates the diff

adoroszlai · 2026-03-29T08:32:10Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

+  private void connectToDatanode(DatanodeDetails dn) throws IOException {
    if (isClosed.get()) {
      throw new IOException("Client is closed.");
    }

    if (isConnected(dn)) {
      return;
    }
-    // read port from the data node, on failure use default configured port
-    int port = dn.getStandalonePort().getValue();
-    if (port == 0) {
-      port = config.getInt(OzoneConfigKeys.HDDS_CONTAINER_IPC_PORT,
-          OzoneConfigKeys.HDDS_CONTAINER_IPC_PORT_DEFAULT);
-    }
-    final int finalPort = port;

-    LOG.debug("Connecting to server : {}; nodes in pipeline : {}, ", dn, pipeline.getNodes());
+    LOG.debug("Connecting to server: {}; nodes in pipeline: {}", dn, pipeline.getNodes());

-    channels.computeIfPresent(dn.getID(), (dnId, channel) -> {
-      if (channel.isTerminated() || channel.isShutdown()) {
-        asyncStubs.remove(dnId);
-        return null; // removes from channels map
-      }
+    removeStaleChannel(dn);
+    generateNewChannel(dn);
+  }


connectToDatanode still performs multiple map operations in a non-atomic way:

calls isConnected twice, once directly, once via removeStaleChannel

isConnected calls containsKey and get separately

removeStaleChannel calls isConnected and remove separately

generateNewChannel calls unconditional put

Use only one compute operation, and distinguish between the three cases in that call:

absent

present but stale

present and active

adoroszlai · 2026-03-29T08:33:46Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

+    private ManagedChannel channel;
+    private XceiverClientProtocolServiceStub stub;


nit: these can be final

ptlrs · 2026-03-29T23:01:25Z

Thanks for the review @adoroszlai, I have pushed the fixes.

yandrey321 · 2026-03-30T17:53:24Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

-        return null; // removes from channels map
+    dnChannelInfoMap.compute(dn.getID(), (dnId, channelInfo) -> {
+      // channel is absent or stale
+      if (channelInfo == null || channelInfo.isChannelInactive()) {


do we need to close inactive channel?

If the channel is inactive, it is already either terminated or shutdown. In both cases the underlying resources have already been released.

adoroszlai

Thanks @ptlrs for updating the patch. compute logic looks good now.

adoroszlai · 2026-04-01T09:27:34Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

+    if (details == null || !dnChannelInfoMap.containsKey(details.getID())) {
+      return false;
+    }

-  private boolean isConnected(ManagedChannel channel) {
-    return channel != null && !channel.isTerminated() && !channel.isShutdown();
+    return !dnChannelInfoMap.get(details.getID()).isChannelInactive();


Avoid separate containsKey and get.

if (details == null) { return false; } ChannelInfo info = dnChannelInfoMap.get(details.getID()); return info != null && !info.isChannelInactive();

adoroszlai · 2026-04-01T09:30:17Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

+        .map(ChannelInfo::getChannel)
+        .collect(Collectors.toList());


ChannelInfo.channel may be null (or at least there are checks for that elsewhere).

Suggested change

.map(ChannelInfo::getChannel)

.collect(Collectors.toList());

.map(ChannelInfo::getChannel)

.filter(Objects::nonNull)

.collect(Collectors.toList());

adoroszlai · 2026-04-01T09:41:06Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

      throw new IOException("This channel is not connected.");
    }

-    ManagedChannel channel = channels.get(dn.getID());
-    // If the channel doesn't exist for this specific datanode or the channel
-    // is closed, just reconnect
-    if (!isConnected(channel)) {
+    // If the channel doesn't exist for this specific datanode or the channel is closed, just reconnect
+    if (!isConnected(dn)) {
      reconnect(dn);
    }
-
  }

  private void reconnect(DatanodeDetails dn)
      throws IOException {
-    ManagedChannel channel;
    try {
      connectToDatanode(dn);
-      channel = channels.get(dn.getID());
    } catch (Exception e) {
      throw new IOException("Error while connecting", e);
    }

-    if (!isConnected(channel)) {
+    if (!isConnected(dn)) {
      throw new IOException("This channel is not connected.");
    }


Given that connectToDatanode handles all cases ("closed", "already connected", "needs new connection"), checkOpen can be simplified:

connectToDatanode(dn); if (!isConnected(dn)) { throw new IOException("This channel is not connected."); }

and reconnect can be removed.

adoroszlai · 2026-04-01T10:00:41Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

-    } catch (RuntimeException e) {
-      LOG.error("Failed to create channel to datanode {}", dn, e);
-      throw new IOException(e.getCause());


We need to keep the translation back from unchecked exception to IOException, because callers may not handle the former.

try { dnChannelInfoMap.compute(...); } catch (UncheckedIOException e) { LOG.error("Failed to create channel to datanode {}", dn, e); throw e.getCause(); }

ptlrs added 4 commits March 27, 2026 11:31

HDDS-14793. Add test to reproduce intermittent NPE in `XceiverClientG…

0cf6828

…rpc#sendCommandAsync`.

HDDS-14793. Fix race condition in `XceiverClientGrpc#connectToDatanod…

bfdcec7

…e` causing intermittent NPEs.

HDDS-14793. Make ChannelInfo static to fix findbugs

cfc88c5

HDDS-14793. Replace Map with ConcurrentMap

30942d7

adoroszlai reviewed Mar 29, 2026

View reviewed changes

HDDS-14793. Fix review comments

e3d0710

ptlrs requested a review from adoroszlai March 30, 2026 16:28

yandrey321 reviewed Mar 30, 2026

View reviewed changes

adoroszlai reviewed Apr 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-14793. Fix race condition in XceiverClientGrpc#connectToDatanode causing intermittent NPEs.#9997

HDDS-14793. Fix race condition in XceiverClientGrpc#connectToDatanode causing intermittent NPEs.#9997
ptlrs wants to merge 5 commits intoapache:masterfrom
ptlrs:HDDS-14793-Intermittent-NPE-in-XceiverClientGrpc

ptlrs commented Mar 29, 2026 •

edited

Loading

Uh oh!

adoroszlai left a comment

Uh oh!

adoroszlai Mar 29, 2026

Uh oh!

adoroszlai Mar 29, 2026

Uh oh!

adoroszlai Mar 29, 2026

Uh oh!

ptlrs commented Mar 29, 2026

Uh oh!

yandrey321 Mar 30, 2026

Uh oh!

ptlrs Mar 30, 2026

Uh oh!

adoroszlai left a comment

Uh oh!

adoroszlai Apr 1, 2026

Uh oh!

adoroszlai Apr 1, 2026

Uh oh!

adoroszlai Apr 1, 2026

Uh oh!

adoroszlai Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		private ManagedChannel channel;
		private XceiverClientProtocolServiceStub stub;

Conversation

ptlrs commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ptlrs commented Mar 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ptlrs commented Mar 29, 2026 •

edited

Loading