Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/connection/Connection.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -92,6 +92,30 @@ 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().
// Guard the teardown itself so a cleanup failure can't mask the
// original error that we rethrow below.
try {
if (this.connection) {
this.connection.removeAllListeners();
if (typeof this.connection.destroy === "function") {
this.connection.destroy();
} else {
this.connection.end();
}
this.connection = null;
}
} catch (cleanupError) {
logger.warn("Error during connection teardown:", cleanupError);
}
this.client = null;
// Mirror close(): clear session/statement ids so a failed setup does
// not leave a stale sessionId reachable via getSessionId().
this.sessionId = null;
this.statementId = null;
this.isConnected = false;
Comment on lines +95 to +118

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.

throw error;
}
}
Expand Down
64 changes: 64 additions & 0 deletions tests/unit/Connection.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -129,4 +129,68 @@ 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);

// The original setup error must surface, not be masked by the teardown.
await expect(connection.open()).rejects.toThrow("auth failed");

// The half-open connection must be torn down (mirrors close()); the buggy
// catch only logged + rethrew, leaking the socket and its listeners.
expect(thriftMock.__mockConnection.removeAllListeners).toHaveBeenCalled();
expect(thriftMock.__mockConnection.destroy).toHaveBeenCalled();
});

test("Should clear sessionId when statement setup fails after openSession", async () => {
// openSession succeeds (sets sessionId), then requestStatementId rejects.
thriftMock.createClient.mockReturnValueOnce({
openSession: jest.fn((_req: unknown, callback: (e: Error | null, r: unknown) => void) =>
callback(null, { status: { code: 200 }, sessionId: 123 }),
),
requestStatementId: jest.fn((_sid: unknown, callback: (e: Error | null, r: unknown) => void) =>
callback(new Error("statement setup failed"), null),
),
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: "root",
enableSSL: false,
sqlDialect: "tree",
};
const connection = new Connection(config);

await expect(connection.open()).rejects.toThrow("statement setup failed");

// The failed setup must not leave a stale sessionId reachable (mirrors
// close()); getSessionId() throws once the id is cleared.
expect(() => connection.getSessionId()).toThrow("Session is not open");
});
});
Loading