Skip to content

Tear down the connection when session setup fails in Connection.open() - #20

Merged
CritasWang merged 2 commits into
apache:developfrom
PDGGK:fix/connection-open-leak
Jul 24, 2026
Merged

Tear down the connection when session setup fails in Connection.open()#20
CritasWang merged 2 commits into
apache:developfrom
PDGGK:fix/connection-open-leak

Conversation

@PDGGK

Copy link
Copy Markdown
Contributor

This is the connection-leak PR (finding 6) from the dev@ discussion "[DISCUSS] Hardening the iotdb-client-nodejs session pool / connection lifecycle".

Problem

Connection.open() establishes the TCP connection (createConnection + createClient) and registers its 'error'/'close' listeners, then performs session setup (openSession, requestStatementId). If session setup rejects (bad credentials, a timeout, or a non-200 status), the catch only logged and rethrew — leaking the open socket and its listeners. Through the pool this propagates out of init() with no close(), so every failed connect attempt leaks one socket.

Fix

Mirror close()'s teardown in the catch before rethrowing: removeAllListeners, destroy/end the socket, null the refs, set isConnected = false.

Tests

Added a regression test asserting teardown when openSession fails after the TCP connect. Full tests/unit suite green (155 tests).

After the TCP connection is established (createConnection + createClient) and
its 'error'/'close' listeners are registered, if openSession() or
requestStatementId() rejects (bad credentials, timeout, or a non-200 status),
the catch only logged the error and rethrew — leaking the open socket and its
listeners. In the pool this propagates through init() with no close(), so each
failed connect attempt leaks a socket.
Mirror close()'s teardown (removeAllListeners + destroy/end + null the refs +
isConnected=false) in the catch before rethrowing, and add a regression test.
Signed-off-by: Zihan Dai <99155080+PDGGK@users.noreply.github.com>

@CritasWangCritasWang 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.

Verified the leak on develop (21b9432): the old catch only logged and rethrew, and neither Session.open() nor the pools' createPoolSession() do any fallback cleanup, so every failed session setup (bad credentials, timeout, non-200) really did strand a live socket plus its 'error'/'close' listeners.

The fix checks out on the edge cases I probed:

  • Socket not yet created (host/port validation or createConnection throwing): the if (this.connection) guard skips cleanly and just rethrows. ✅
  • Double-teardown safety: the registered 'error'/'close' listeners only flip isConnected, they never destroy, and socket.destroy() is idempotent — no double-destroy hazard. ✅
  • Timers: the 30s timeouts inside openSession/requestStatementId clear themselves in their callbacks, and Connection has no heartbeat/interval, so nothing else needs cleanup here. ✅

The regression test is valid — the base catch never calls removeAllListeners/destroy, so it fails on develop and passes here.

Two low-severity suggestions inline (stale sessionId not reset, and guarding the teardown so it can't mask the original error). One style note, no change requested: this teardown block now exists in three places (close() happy path, close() catch, open() catch) — a private teardownConnection() helper would be a nice follow-up, either here or later.

Looks good to merge with or without the inline tweaks.

Comment on lines +95 to +108
// Tear down the half-open connection so its socket and event listeners
// don't leak when session setup (openSession/requestStatementId) fails
// after the TCP connection was already established. Mirrors close().
if (this.connection) {
this.connection.removeAllListeners();
if (typeof this.connection.destroy === "function") {
this.connection.destroy();
} else {
this.connection.end();
}
this.connection = null;
}
this.client = null;
this.isConnected = false;

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.

Two small things to fully "mirror close()" here:

  1. close() also nulls sessionId (and statementId), but this catch doesn't. If openSession succeeds and then requestStatementId fails, the object keeps a stale sessionId — harmless for isOpen()/re-close(), but getSessionId() would still return the dead session's id. Worth resetting both here for consistency.

  2. The teardown itself isn't guarded: if removeAllListeners()/destroy() ever throws, it would replace the original error (e.g. the auth failure) as the propagated one. Wrapping the teardown in a try { … } catch { /* log */ } keeps the throw error below always rethrowing the real cause. close() has an outer catch that plays this role; this path doesn't.

Comment threadtests/unit/Connection.test.ts Outdated
};
const connection = new Connection(config);

await expect(connection.open()).rejects.toThrow();

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.

Nit: consider asserting the original error surfaces, e.g. .rejects.toThrow("auth failed"). That pins down that the new teardown never masks the real failure cause (see the comment on the source side), and makes the regression test a bit stronger for free.

Address review feedback on apache#20: on a failed session setup the open()
catch now (1) clears sessionId/statementId so getSessionId() cannot return
a stale id after openSession succeeds but requestStatementId fails, and
(2) wraps the socket teardown in its own try/catch so a cleanup failure
cannot mask the original error that is rethrown below.
Strengthen the regression tests: assert the original setup error surfaces
rather than a teardown error, and add a case asserting that a failure
after openSession clears the session id.
Signed-off-by: Zihan Dai <99155080+PDGGK@users.noreply.github.com>
@PDGGK

Copy link
Copy Markdown
ContributorAuthor

Thanks for the thorough review @CritasWang — applied both inline suggestions:

  • open()'s catch now clears sessionId (as close() does) and statementId too, per your suggestion, so a failure after openSession succeeds no longer leaves a stale id reachable via getSessionId().
  • The teardown is wrapped in its own try/catch, so a cleanup failure can't replace the original setup error that's rethrown.

Tests: strengthened the existing case to assert the original error surfaces (.rejects.toThrow("auth failed")), and added one for the post-openSession failure clearing the session id.

On the teardownConnection() helper — agreed, the teardown now appears in three places. Happy to extract it, either here or as a small follow-up PR, whichever you'd prefer.

@CritasWang
CritasWang merged commit df7c891 into apache:developJul 24, 2026
4 checks passed
Sign up for freeto 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

@PDGGK@CritasWang