CritasWang commented on code in PR #20:
URL:
https://github.com/apache/iotdb-client-nodejs/pull/20#discussion_r3635191345
##########
src/connection/Connection.ts:
##########
@@ -92,6 +92,20 @@ export class Connection {
this.isConnected = true;
} catch (error) {
logger.error("Failed to connect:", error);
+ // 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;
Review Comment:
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.
##########
tests/unit/Connection.test.ts:
##########
@@ -129,4 +129,36 @@ describe("Connection", () => {
await connection.close();
});
+
+ test("Should tear down the socket when session setup fails", async () => {
+ // openSession rejects after the TCP connection was established.
+ thriftMock.createClient.mockReturnValueOnce({
+ openSession: jest.fn((_req: unknown, callback: (e: Error | null, r:
unknown) => void) =>
+ callback(new Error("auth failed"), null),
+ ),
+ requestStatementId: jest.fn((_sid: unknown, callback: (e: Error | null,
r: unknown) => void) =>
+ callback(null, 456),
+ ),
+ closeSession: jest.fn((_req: unknown, callback: (e: Error | null, r:
unknown) => void) =>
+ callback(null, { status: { code: 200 } }),
+ ),
+ });
+
+ const config: InternalConfig = {
+ host: "localhost",
+ port: 6667,
+ username: "root",
+ password: "bad",
+ enableSSL: false,
+ sqlDialect: "tree",
+ };
+ const connection = new Connection(config);
+
+ await expect(connection.open()).rejects.toThrow();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
