Skip to content

[Session] Throw IoTDBConnectionException instead of NPE when retries are exhausted - #18794

Open
yamass wants to merge 1 commit into
apache:masterfrom
yamass:fix/session-npe-on-exhausted-retries
Open

yamass wants to merge 1 commit into
apache:masterfrom
yamass:fix/session-npe-on-exhausted-retries

Conversation

@yamass

@yamass yamass commented Oct 6, 2026

Copy link
Copy Markdown

Description

Problem

When every attempt in SessionConnection.callWithRetryAndReconnect fails with a TException, it returns a RetryResult with a null result. Its callers (queries, TSStatus methods, schema template methods) use the result without checking it and throw a NullPointerException:

NullPointerException: Cannot invoke "TSExecuteStatementResp.getStatus()" because "execResp" is null
  at SessionConnection.executeQueryStatement(...)

As a result:

  • The real cause (for example TTransportException: Cannot write to null outputStream) is lost.
  • SessionPool handles the NPE in its RuntimeException branch and puts the broken session back at the head of its queue, so the next call gets the same session. As long as that session cannot reconnect, each call fails the same way. We saw this continue until the client JVM was restarted.

Fix

callWithRetryAndReconnect now throws IoTDBConnectionException(lastTException) when no attempt produced a result. The result can only be null if the last attempt threw, so the cause is always set.

  • The check is in the shared helper, so it covers all callers without changing any of them.
  • SessionPool now takes its existing IoTDBConnectionException branch, which closes the broken session and creates a new one.
  • No new message strings, so no i18n changes. I don't see a reason to add a message where the exception is thrown.

Tests

New BrokenSessionConnectionTest uses a real, closed Thrift transport and an unreachable reconnect target. It checks that executeQueryStatement and setStorageGroup throw IoTDBConnectionException with the cause attached, and that a broken connection leads to a session eviction. These tests fail on master and pass with this change. All iotdb-session unit tests pass.


This PR has:

  • been self-reviewed.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage.

Key changed/added classes (or packages if there are too many classes) in this PR
  • iotdb-client/session: SessionConnection, BrokenSessionConnectionTest (new)

…are exhausted

callWithRetryAndReconnect returned a null result after all attempts failed
with a TException. Callers dereferenced it and threw a NullPointerException,
losing the real cause. SessionPool treated the NPE as a RuntimeException and
put the broken session back into the pool, so every later call failed the
same way until the client JVM was restarted.

Throw IoTDBConnectionException with the last TException as the cause, so the
error is diagnosable and SessionPool evicts the broken session.
@yamass
yamass force-pushed the fix/session-npe-on-exhausted-retries branch from 77a0ce3 to 86b842c Compare October 7, 2026 10:30
@HTHou
HTHou requested a balanced review from Copilot October 7, 2026 12:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The shared fix correctly preserves the root cause and is covered across direct and pooled session paths.

Review effort: Balanced
Findings: None

What changed in this PR

Ensures exhausted Session RPC retries preserve the underlying Thrift failure as an IoTDBConnectionException.

Changes:

  • Throws a connection exception when retries produce no result.
  • Adds query, status-path, and pool-eviction regression tests.
File Description
SessionConnection.java Converts exhausted retry failures into connection exceptions.
BrokenSessionConnectionTest.java Tests cause preservation and broken-session eviction.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants