From 1b86bd436c2aea83a7a7f14a18e2f10bf699b1b7 Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 10 Jun 2026 00:18:07 +0000 Subject: [PATCH 1/2] sql: don't let a throwing onconnect/onclose callback corrupt the connection pool The user-provided onconnect/onclose callbacks ran before the pooled connection handlers updated their own bookkeeping. If the callback threw, the handler aborted early: state stayed pending, storedError was never recorded, pending queries were never notified, onFinish never ran and the connection was never released, so queries, connect() and end() hung forever. Run the callback in a try/finally so the bookkeeping always completes; the exception still propagates and is reported as an uncaughtException, as before. Also defer the synchronous onClose call in postgres createConnection's catch with process.nextTick, matching mysql. It could fire while the adapter was still filling this.connections, so pool methods that scan that array (flush, isConnected, close) threw a TypeError on the holes when called from inside the onclose callback. Fixes #32037 --- src/js/internal/sql/mysql.ts | 93 ++++--- src/js/internal/sql/postgres.ts | 101 ++++--- .../sql/sql-onconnect-onclose-throw.test.ts | 252 ++++++++++++++++++ 3 files changed, 361 insertions(+), 85 deletions(-) create mode 100644 test/js/sql/sql-onconnect-onclose-throw.test.ts diff --git a/src/js/internal/sql/mysql.ts b/src/js/internal/sql/mysql.ts index 4786bfc84570..625941940449 100644 --- a/src/js/internal/sql/mysql.ts +++ b/src/js/internal/sql/mysql.ts @@ -319,31 +319,35 @@ class PooledMySQLConnection { } const connectionInfo = this.connectionInfo; - if (connectionInfo?.onconnect) { - connectionInfo.onconnect(err); - } - this.storedError = err; - if (!err) { - this.connectStartedAt = 0; - this.flags |= PooledConnectionFlags.canBeConnected; - } - this.state = err ? PooledConnectionState.closed : PooledConnectionState.connected; - const onFinish = this.onFinish; - if (onFinish) { - this.queryCount = 0; - this.flags &= ~PooledConnectionFlags.reserved; - this.flags &= ~PooledConnectionFlags.preReserved; - - // pool is closed, lets finish the connection - // pool is closed, lets finish the connection - if (err) { - onFinish(err); + try { + // user code; a throw must not abort the pool bookkeeping below + // (the exception keeps propagating after the finally block runs) + if (connectionInfo?.onconnect) { + connectionInfo.onconnect(err); + } + } finally { + this.storedError = err; + if (!err) { + this.connectStartedAt = 0; + this.flags |= PooledConnectionFlags.canBeConnected; + } + this.state = err ? PooledConnectionState.closed : PooledConnectionState.connected; + const onFinish = this.onFinish; + if (onFinish) { + this.queryCount = 0; + this.flags &= ~PooledConnectionFlags.reserved; + this.flags &= ~PooledConnectionFlags.preReserved; + + // pool is closed, lets finish the connection + if (err) { + onFinish(err); + } else { + this.connection?.close(); + } } else { - this.connection?.close(); + this.adapter.release(this, true); } - return; } - this.adapter.release(this, true); } #onClose(err) { @@ -380,29 +384,34 @@ class PooledMySQLConnection { #finishClose(err) { const connectionInfo = this.connectionInfo; - if (connectionInfo?.onclose) { - connectionInfo.onclose(err); - } - this.state = PooledConnectionState.closed; - this.storedError = err; + try { + // user code; a throw must not abort the pool bookkeeping below + // (the exception keeps propagating after the finally block runs) + if (connectionInfo?.onclose) { + connectionInfo.onclose(err); + } + } finally { + this.state = PooledConnectionState.closed; + this.storedError = err; + + // remove from ready connections if its there + this.adapter.readyConnections.delete(this); + const queries = new Set(this.queries); + this.queries?.clear?.(); + this.queryCount = 0; + this.flags &= ~PooledConnectionFlags.reserved; - // remove from ready connections if its there - this.adapter.readyConnections.delete(this); - const queries = new Set(this.queries); - this.queries?.clear?.(); - this.queryCount = 0; - this.flags &= ~PooledConnectionFlags.reserved; + // notify all queries that the connection is closed + for (const onClose of queries) { + onClose(err); + } + const onFinish = this.onFinish; + if (onFinish) { + onFinish(err); + } - // notify all queries that the connection is closed - for (const onClose of queries) { - onClose(err); - } - const onFinish = this.onFinish; - if (onFinish) { - onFinish(err); + this.adapter.release(this, true); } - - this.adapter.release(this, true); } constructor(connectionInfo: Bun.SQL.__internal.DefinedMySQLOptions, adapter: MySQLAdapter) { diff --git a/src/js/internal/sql/postgres.ts b/src/js/internal/sql/postgres.ts index 655f2509f189..d778b6e0687d 100644 --- a/src/js/internal/sql/postgres.ts +++ b/src/js/internal/sql/postgres.ts @@ -461,6 +461,10 @@ function onQueryFinish(this: PooledPostgresConnection, onClose: (err: Error) => this.adapter.release(this); } +function closeNT(onClose: (err: Error) => void, err: Error | null) { + onClose(err as Error); +} + class PooledPostgresConnection { private static async createConnection( options: Bun.SQL.__internal.DefinedPostgresOrMySQLOptions, @@ -515,7 +519,9 @@ class PooledPostgresConnection { !prepare, ); } catch (e) { - onClose(e as Error); + // defer so the callback never runs while the adapter is still filling + // this.connections (it scans that array); mysql.ts does the same + process.nextTick(closeNT, onClose, e); return null; } } @@ -542,31 +548,35 @@ class PooledPostgresConnection { err = wrapPostgresError(err); } const connectionInfo = this.connectionInfo; - if (connectionInfo?.onconnect) { - connectionInfo.onconnect(err); - } - this.storedError = err; - if (!err) { - this.connectStartedAt = 0; - this.flags |= PooledConnectionFlags.canBeConnected; - } - this.state = err ? PooledConnectionState.closed : PooledConnectionState.connected; - const onFinish = this.onFinish; - if (onFinish) { - this.queryCount = 0; - this.flags &= ~PooledConnectionFlags.reserved; - this.flags &= ~PooledConnectionFlags.preReserved; - - // pool is closed, lets finish the connection - // pool is closed, lets finish the connection - if (err) { - onFinish(err); + try { + // user code; a throw must not abort the pool bookkeeping below + // (the exception keeps propagating after the finally block runs) + if (connectionInfo?.onconnect) { + connectionInfo.onconnect(err); + } + } finally { + this.storedError = err; + if (!err) { + this.connectStartedAt = 0; + this.flags |= PooledConnectionFlags.canBeConnected; + } + this.state = err ? PooledConnectionState.closed : PooledConnectionState.connected; + const onFinish = this.onFinish; + if (onFinish) { + this.queryCount = 0; + this.flags &= ~PooledConnectionFlags.reserved; + this.flags &= ~PooledConnectionFlags.preReserved; + + // pool is closed, lets finish the connection + if (err) { + onFinish(err); + } else { + this.connection?.close(); + } } else { - this.connection?.close(); + this.adapter.release(this, true); } - return; } - this.adapter.release(this, true); } #onClose(err) { @@ -603,29 +613,34 @@ class PooledPostgresConnection { #finishClose(err) { const connectionInfo = this.connectionInfo; - if (connectionInfo?.onclose) { - connectionInfo.onclose(err); - } - this.state = PooledConnectionState.closed; - this.storedError = err; + try { + // user code; a throw must not abort the pool bookkeeping below + // (the exception keeps propagating after the finally block runs) + if (connectionInfo?.onclose) { + connectionInfo.onclose(err); + } + } finally { + this.state = PooledConnectionState.closed; + this.storedError = err; + + // remove from ready connections if its there + this.adapter.readyConnections?.delete(this); + const queries = new Set(this.queries); + this.queries?.clear?.(); + this.queryCount = 0; + this.flags &= ~PooledConnectionFlags.reserved; - // remove from ready connections if its there - this.adapter.readyConnections?.delete(this); - const queries = new Set(this.queries); - this.queries?.clear?.(); - this.queryCount = 0; - this.flags &= ~PooledConnectionFlags.reserved; + // notify all queries that the connection is closed + for (const onClose of queries) { + onClose(err); + } + const onFinish = this.onFinish; + if (onFinish) { + onFinish(err); + } - // notify all queries that the connection is closed - for (const onClose of queries) { - onClose(err); - } - const onFinish = this.onFinish; - if (onFinish) { - onFinish(err); + this.adapter.release(this, true); } - - this.adapter.release(this, true); } constructor(connectionInfo: Bun.SQL.__internal.DefinedPostgresOrMySQLOptions, adapter: PostgresAdapter) { diff --git a/test/js/sql/sql-onconnect-onclose-throw.test.ts b/test/js/sql/sql-onconnect-onclose-throw.test.ts new file mode 100644 index 000000000000..699b3cd2c087 --- /dev/null +++ b/test/js/sql/sql-onconnect-onclose-throw.test.ts @@ -0,0 +1,252 @@ +// A user-provided onconnect/onclose callback that throws used to abort the +// pool's connection handler mid-way: the connection state stayed pending, +// storedError was never recorded, pending queries were never notified and +// release() never ran, so anything awaiting the pool (queries, connect(), +// end()) hung forever. The callback exception must not abort the pool +// bookkeeping; it still surfaces as an uncaughtException. +// https://github.com/oven-sh/bun/issues/32037 +// +// Uses mock servers / closed ports so the tests run without Docker. Each +// scenario runs in a subprocess because the throwing callback is reported as +// a process-level uncaughtException. + +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; + +async function runFixture(code: string) { + using dir = tempDir("sql-throwing-hooks", { "fixture.ts": code }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.ts"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; +} + +// Minimal postgres handshake: reply to the startup message with +// AuthenticationOk + ReadyForQuery, then ignore everything else. +const pgMockServer = /* ts */ ` +const net = require("net"); +function startServer() { + const server = net.createServer(socket => { + let handshakeDone = false; + socket.on("data", () => { + if (handshakeDone) return; + handshakeDone = true; + const authOk = Buffer.alloc(9); + authOk.write("R", 0); + authOk.writeInt32BE(8, 1); + authOk.writeInt32BE(0, 5); + const ready = Buffer.alloc(6); + ready.write("Z", 0); + ready.writeInt32BE(5, 1); + ready.write("I", 5); + socket.write(Buffer.concat([authOk, ready])); + }); + socket.on("error", () => {}); + }); + return new Promise(resolve => { + server.listen(0, "127.0.0.1", () => resolve(server.address().port)); + }); +} +`; + +// Minimal mysql handshake: HandshakeV10, then an OK packet for the +// handshake response, then ignore everything else. +const mysqlMockServer = /* ts */ ` +const net = require("net"); +function u16le(n) { return Buffer.from([n & 0xff, (n >> 8) & 0xff]); } +function u24le(n) { return Buffer.from([n & 0xff, (n >> 8) & 0xff, (n >> 16) & 0xff]); } +function u32le(n) { return Buffer.from([n & 0xff, (n >> 8) & 0xff, (n >> 16) & 0xff, (n >>> 24) & 0xff]); } +function packet(seq, payload) { return Buffer.concat([u24le(payload.length), Buffer.from([seq]), payload]); } +const SERVER_CAPS = (1 << 9) | (1 << 15) | (1 << 19) | (1 << 21) | (1 << 24); +function handshakeV10() { + const authData1 = Buffer.alloc(8, 0x61); + const authData2 = Buffer.alloc(13, 0x62); + authData2[12] = 0; + return packet(0, Buffer.concat([ + Buffer.from([10]), Buffer.from("mock-5.7.0\\0"), u32le(1), authData1, + Buffer.from([0]), u16le(SERVER_CAPS & 0xffff), Buffer.from([0x2d]), + u16le(0x0002), u16le((SERVER_CAPS >>> 16) & 0xffff), Buffer.from([21]), + Buffer.alloc(10, 0), authData2, Buffer.from("mysql_native_password\\0"), + ])); +} +function okPacket(seq) { return packet(seq, Buffer.from([0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00])); } +function startServer() { + const server = net.createServer(socket => { + let buffered = Buffer.alloc(0), authed = false; + socket.write(handshakeV10()); + socket.on("data", chunk => { + buffered = Buffer.concat([buffered, chunk]); + while (buffered.length >= 4) { + const len = buffered[0] | (buffered[1] << 8) | (buffered[2] << 16); + if (buffered.length < 4 + len) break; + const seq = buffered[3]; + buffered = buffered.subarray(4 + len); + if (!authed) { authed = true; socket.write(okPacket(seq + 1)); } + } + }); + socket.on("error", () => {}); + }); + return new Promise(resolve => { + server.listen(0, "127.0.0.1", () => resolve(server.address().port)); + }); +} +`; + +// A port with nothing listening on it. +const closedPort = /* ts */ ` +const net = require("net"); +function closedPort() { + return new Promise(resolve => { + const server = net.createServer(); + server.listen(0, "127.0.0.1", () => { + const port = server.address().port; + server.close(() => resolve(port)); + }); + }); +} +`; + +function connectAndEnd(adapter: "postgres" | "mysql", hook: "onconnect" | "onclose") { + const url = adapter === "postgres" ? "postgres://postgres@127.0.0.1:" : "mysql://root@127.0.0.1:"; + const db = adapter === "postgres" ? "/postgres" : "/db"; + return ( + (adapter === "postgres" ? pgMockServer : mysqlMockServer) + + /* ts */ ` +import { SQL } from "bun"; +process.on("uncaughtException", err => console.log("uncaught:", err.message)); +const port = await startServer(); +const sql = new SQL({ + url: "${url}" + port + "${db}", + max: 1, + ${hook}(err) { + console.log("${hook}:", err === null || err === undefined ? null : err.message); + throw new Error("boom from ${hook}"); + }, +}); +await sql.connect(); +console.log("connected"); +await sql.end(); +console.log("ended"); +process.exit(0); +` + ); +} + +function failToConnect(adapter: "postgres" | "mysql") { + const url = adapter === "postgres" ? "postgres://postgres@127.0.0.1:" : "mysql://root@127.0.0.1:"; + const db = adapter === "postgres" ? "/postgres" : "/db"; + return ( + closedPort + + /* ts */ ` +import { SQL } from "bun"; +process.on("uncaughtException", err => console.log("uncaught:", err.message)); +const port = await closedPort(); +const sql = new SQL({ + url: "${url}" + port + "${db}", + max: 1, + onclose(err) { + console.log("onclose:", err.code); + throw new Error("boom from onclose"); + }, +}); +try { + await sql.unsafe("SELECT 1"); + console.log("query resolved"); +} catch (err) { + console.log("query rejected:", err.code); +} +process.exit(0); +` + ); +} + +test.concurrent("postgres: a throwing onconnect callback does not leave the pool stuck", async () => { + const { stdout, exitCode } = await runFixture(connectAndEnd("postgres", "onconnect")); + expect(stdout).toBe("onconnect: null\nuncaught: boom from onconnect\nconnected\nended\n"); + expect(exitCode).toBe(0); +}); + +test.concurrent("mysql: a throwing onconnect callback does not leave the pool stuck", async () => { + const { stdout, exitCode } = await runFixture(connectAndEnd("mysql", "onconnect")); + expect(stdout).toBe("onconnect: null\nuncaught: boom from onconnect\nconnected\nended\n"); + expect(exitCode).toBe(0); +}); + +test.concurrent("postgres: a throwing onclose callback does not hang sql.end()", async () => { + const { stdout, exitCode } = await runFixture(connectAndEnd("postgres", "onclose")); + expect(stdout).toBe("connected\nonclose: Connection closed\nuncaught: boom from onclose\nended\n"); + expect(exitCode).toBe(0); +}); + +test.concurrent("mysql: a throwing onclose callback does not hang sql.end()", async () => { + const { stdout, exitCode } = await runFixture(connectAndEnd("mysql", "onclose")); + expect(stdout).toBe("connected\nonclose: Connection closed\nuncaught: boom from onclose\nended\n"); + expect(exitCode).toBe(0); +}); + +test.concurrent( + "postgres: a throwing onclose callback still rejects pending queries when the connection is refused", + async () => { + const { stdout, exitCode } = await runFixture(failToConnect("postgres")); + expect(stdout).toBe( + "onclose: ERR_POSTGRES_CONNECTION_REFUSED\nuncaught: boom from onclose\nquery rejected: ERR_POSTGRES_CONNECTION_REFUSED\n", + ); + expect(exitCode).toBe(0); + }, +); + +test.concurrent( + "mysql: a throwing onclose callback still rejects pending queries when the connection is refused", + async () => { + const { stdout, exitCode } = await runFixture(failToConnect("mysql")); + expect(stdout).toBe( + "onclose: ERR_MYSQL_CONNECTION_REFUSED\nuncaught: boom from onclose\nquery rejected: ERR_MYSQL_CONNECTION_REFUSED\n", + ); + expect(exitCode).toBe(0); + }, +); + +// When createConnection fails synchronously (here: a password function that +// throws), onclose used to be invoked while the adapter was still filling +// this.connections, so pool methods that scan that array (flush, isConnected, +// close) threw a TypeError on the holes when called from inside the callback. +// The callback is now deferred until the pool is fully constructed. +test.concurrent("postgres: pool calls from onclose are safe when connecting fails synchronously", async () => { + const fixture = /* ts */ ` +import { SQL } from "bun"; +process.on("uncaughtException", err => console.log("uncaught:", err.message)); +const sql = new SQL({ + adapter: "postgres", + hostname: "127.0.0.1", + port: 1, // never dialed: password() throws before the connection is created + username: "postgres", + database: "postgres", + max: 2, + password: () => { + throw new Error("password error"); + }, + onclose(err) { + try { + sql.flush(); + console.log("reentry ok"); + } catch (err2) { + console.log("reentry threw:", err2.constructor.name); + } + }, +}); +try { + await sql.unsafe("SELECT 1"); + console.log("query resolved"); +} catch (err) { + console.log("query rejected:", err.message); +} +process.exit(0); +`; + const { stdout, exitCode } = await runFixture(fixture); + expect(stdout).toBe("reentry ok\nreentry ok\nquery rejected: password error\n"); + expect(exitCode).toBe(0); +}); From 7139c12d2fc6d479407b841c55b0fe403b0af7c2 Mon Sep 17 00:00:00 2001 From: robobun Date: Wed, 10 Jun 2026 22:50:34 +0000 Subject: [PATCH 2/2] test: run the throwing-hook success paths against real docker postgres/mysql Replace the mock protocol servers with the docker-compose postgres_plain and mysql_plain services via describeWithContainer, matching the other sql tests. The connection-refused tests keep using a real closed port and the synchronous-failure test never dials, so those still run without docker. --- .../sql/sql-onconnect-onclose-throw.test.ts | 196 +++++++----------- 1 file changed, 71 insertions(+), 125 deletions(-) diff --git a/test/js/sql/sql-onconnect-onclose-throw.test.ts b/test/js/sql/sql-onconnect-onclose-throw.test.ts index 699b3cd2c087..35d91b093156 100644 --- a/test/js/sql/sql-onconnect-onclose-throw.test.ts +++ b/test/js/sql/sql-onconnect-onclose-throw.test.ts @@ -6,18 +6,20 @@ // bookkeeping; it still surfaces as an uncaughtException. // https://github.com/oven-sh/bun/issues/32037 // -// Uses mock servers / closed ports so the tests run without Docker. Each -// scenario runs in a subprocess because the throwing callback is reported as -// a process-level uncaughtException. +// The established-connection scenarios run against the real docker-compose +// postgres/mysql services. The connection-refused scenarios use a real closed +// port and the synchronous-failure scenario never dials, so those run +// everywhere. Each scenario runs in a subprocess because the throwing +// callback is reported as a process-level uncaughtException. import { expect, test } from "bun:test"; -import { bunEnv, bunExe, tempDir } from "harness"; +import { bunEnv, bunExe, describeWithContainer, isDockerEnabled, tempDir } from "harness"; -async function runFixture(code: string) { +async function runFixture(code: string, env: Record = {}) { using dir = tempDir("sql-throwing-hooks", { "fixture.ts": code }); await using proc = Bun.spawn({ cmd: [bunExe(), "fixture.ts"], - env: bunEnv, + env: { ...bunEnv, ...env }, cwd: String(dir), stderr: "pipe", }); @@ -25,78 +27,71 @@ async function runFixture(code: string) { return { stdout, stderr, exitCode }; } -// Minimal postgres handshake: reply to the startup message with -// AuthenticationOk + ReadyForQuery, then ignore everything else. -const pgMockServer = /* ts */ ` -const net = require("net"); -function startServer() { - const server = net.createServer(socket => { - let handshakeDone = false; - socket.on("data", () => { - if (handshakeDone) return; - handshakeDone = true; - const authOk = Buffer.alloc(9); - authOk.write("R", 0); - authOk.writeInt32BE(8, 1); - authOk.writeInt32BE(0, 5); - const ready = Buffer.alloc(6); - ready.write("Z", 0); - ready.writeInt32BE(5, 1); - ready.write("I", 5); - socket.write(Buffer.concat([authOk, ready])); - }); - socket.on("error", () => {}); - }); - return new Promise(resolve => { - server.listen(0, "127.0.0.1", () => resolve(server.address().port)); - }); -} +// Connects to the server at FIXTURE_URL with a throwing hook installed, runs +// a query, then closes the pool. Without the fix the query (throwing +// onconnect) or sql.end() (throwing onclose) never settles and the fixture +// never reaches "ended". +function throwingHookFixture(hook: "onconnect" | "onclose") { + return /* ts */ ` +import { SQL } from "bun"; +process.on("uncaughtException", err => console.log("uncaught:", err.message)); +const sql = new SQL({ + url: process.env.FIXTURE_URL, + max: 1, + ${hook}(err) { + console.log("${hook}:", err === null || err === undefined ? null : err.message); + throw new Error("boom from ${hook}"); + }, +}); +const rows = await sql.unsafe("SELECT 1 as x"); +console.log("query:", JSON.stringify(rows)); +await sql.end(); +console.log("ended"); +process.exit(0); `; - -// Minimal mysql handshake: HandshakeV10, then an OK packet for the -// handshake response, then ignore everything else. -const mysqlMockServer = /* ts */ ` -const net = require("net"); -function u16le(n) { return Buffer.from([n & 0xff, (n >> 8) & 0xff]); } -function u24le(n) { return Buffer.from([n & 0xff, (n >> 8) & 0xff, (n >> 16) & 0xff]); } -function u32le(n) { return Buffer.from([n & 0xff, (n >> 8) & 0xff, (n >> 16) & 0xff, (n >>> 24) & 0xff]); } -function packet(seq, payload) { return Buffer.concat([u24le(payload.length), Buffer.from([seq]), payload]); } -const SERVER_CAPS = (1 << 9) | (1 << 15) | (1 << 19) | (1 << 21) | (1 << 24); -function handshakeV10() { - const authData1 = Buffer.alloc(8, 0x61); - const authData2 = Buffer.alloc(13, 0x62); - authData2[12] = 0; - return packet(0, Buffer.concat([ - Buffer.from([10]), Buffer.from("mock-5.7.0\\0"), u32le(1), authData1, - Buffer.from([0]), u16le(SERVER_CAPS & 0xffff), Buffer.from([0x2d]), - u16le(0x0002), u16le((SERVER_CAPS >>> 16) & 0xffff), Buffer.from([21]), - Buffer.alloc(10, 0), authData2, Buffer.from("mysql_native_password\\0"), - ])); } -function okPacket(seq) { return packet(seq, Buffer.from([0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00])); } -function startServer() { - const server = net.createServer(socket => { - let buffered = Buffer.alloc(0), authed = false; - socket.write(handshakeV10()); - socket.on("data", chunk => { - buffered = Buffer.concat([buffered, chunk]); - while (buffered.length >= 4) { - const len = buffered[0] | (buffered[1] << 8) | (buffered[2] << 16); - if (buffered.length < 4 + len) break; - const seq = buffered[3]; - buffered = buffered.subarray(4 + len); - if (!authed) { authed = true; socket.write(okPacket(seq + 1)); } - } + +if (isDockerEnabled()) { + describeWithContainer("postgres", { image: "postgres_plain" }, container => { + test("a throwing onconnect callback does not leave the pool stuck", async () => { + await container.ready; + const url = `postgres://bun_sql_test@${container.host}:${container.port}/bun_sql_test`; + const { stdout, exitCode } = await runFixture(throwingHookFixture("onconnect"), { FIXTURE_URL: url }); + expect(stdout).toBe('onconnect: null\nuncaught: boom from onconnect\nquery: [{"x":1}]\nended\n'); + expect(exitCode).toBe(0); + }); + + test("a throwing onclose callback does not hang sql.end()", async () => { + await container.ready; + const url = `postgres://bun_sql_test@${container.host}:${container.port}/bun_sql_test`; + const { stdout, exitCode } = await runFixture(throwingHookFixture("onclose"), { FIXTURE_URL: url }); + expect(stdout).toBe('query: [{"x":1}]\nonclose: Connection closed\nuncaught: boom from onclose\nended\n'); + expect(exitCode).toBe(0); }); - socket.on("error", () => {}); }); - return new Promise(resolve => { - server.listen(0, "127.0.0.1", () => resolve(server.address().port)); + + describeWithContainer("mysql", { image: "mysql_plain" }, container => { + test("a throwing onconnect callback does not leave the pool stuck", async () => { + await container.ready; + const url = `mysql://root@${container.host}:${container.port}/bun_sql_test`; + const { stdout, exitCode } = await runFixture(throwingHookFixture("onconnect"), { FIXTURE_URL: url }); + expect(stdout).toBe('onconnect: null\nuncaught: boom from onconnect\nquery: [{"x":1}]\nended\n'); + expect(exitCode).toBe(0); + }); + + test("a throwing onclose callback does not hang sql.end()", async () => { + await container.ready; + const url = `mysql://root@${container.host}:${container.port}/bun_sql_test`; + const { stdout, exitCode } = await runFixture(throwingHookFixture("onclose"), { FIXTURE_URL: url }); + expect(stdout).toBe('query: [{"x":1}]\nonclose: Connection closed\nuncaught: boom from onclose\nended\n'); + expect(exitCode).toBe(0); + }); }); } -`; -// A port with nothing listening on it. +// A port with nothing listening on it, so the connection is refused. Refused +// connections fail fast (not retried), so the throwing onclose fires on the +// first attempt; without the fix the pending query is never rejected. const closedPort = /* ts */ ` const net = require("net"); function closedPort() { @@ -110,33 +105,7 @@ function closedPort() { } `; -function connectAndEnd(adapter: "postgres" | "mysql", hook: "onconnect" | "onclose") { - const url = adapter === "postgres" ? "postgres://postgres@127.0.0.1:" : "mysql://root@127.0.0.1:"; - const db = adapter === "postgres" ? "/postgres" : "/db"; - return ( - (adapter === "postgres" ? pgMockServer : mysqlMockServer) + - /* ts */ ` -import { SQL } from "bun"; -process.on("uncaughtException", err => console.log("uncaught:", err.message)); -const port = await startServer(); -const sql = new SQL({ - url: "${url}" + port + "${db}", - max: 1, - ${hook}(err) { - console.log("${hook}:", err === null || err === undefined ? null : err.message); - throw new Error("boom from ${hook}"); - }, -}); -await sql.connect(); -console.log("connected"); -await sql.end(); -console.log("ended"); -process.exit(0); -` - ); -} - -function failToConnect(adapter: "postgres" | "mysql") { +function refusedConnectionFixture(adapter: "postgres" | "mysql") { const url = adapter === "postgres" ? "postgres://postgres@127.0.0.1:" : "mysql://root@127.0.0.1:"; const db = adapter === "postgres" ? "/postgres" : "/db"; return ( @@ -164,34 +133,10 @@ process.exit(0); ); } -test.concurrent("postgres: a throwing onconnect callback does not leave the pool stuck", async () => { - const { stdout, exitCode } = await runFixture(connectAndEnd("postgres", "onconnect")); - expect(stdout).toBe("onconnect: null\nuncaught: boom from onconnect\nconnected\nended\n"); - expect(exitCode).toBe(0); -}); - -test.concurrent("mysql: a throwing onconnect callback does not leave the pool stuck", async () => { - const { stdout, exitCode } = await runFixture(connectAndEnd("mysql", "onconnect")); - expect(stdout).toBe("onconnect: null\nuncaught: boom from onconnect\nconnected\nended\n"); - expect(exitCode).toBe(0); -}); - -test.concurrent("postgres: a throwing onclose callback does not hang sql.end()", async () => { - const { stdout, exitCode } = await runFixture(connectAndEnd("postgres", "onclose")); - expect(stdout).toBe("connected\nonclose: Connection closed\nuncaught: boom from onclose\nended\n"); - expect(exitCode).toBe(0); -}); - -test.concurrent("mysql: a throwing onclose callback does not hang sql.end()", async () => { - const { stdout, exitCode } = await runFixture(connectAndEnd("mysql", "onclose")); - expect(stdout).toBe("connected\nonclose: Connection closed\nuncaught: boom from onclose\nended\n"); - expect(exitCode).toBe(0); -}); - test.concurrent( "postgres: a throwing onclose callback still rejects pending queries when the connection is refused", async () => { - const { stdout, exitCode } = await runFixture(failToConnect("postgres")); + const { stdout, exitCode } = await runFixture(refusedConnectionFixture("postgres")); expect(stdout).toBe( "onclose: ERR_POSTGRES_CONNECTION_REFUSED\nuncaught: boom from onclose\nquery rejected: ERR_POSTGRES_CONNECTION_REFUSED\n", ); @@ -202,7 +147,7 @@ test.concurrent( test.concurrent( "mysql: a throwing onclose callback still rejects pending queries when the connection is refused", async () => { - const { stdout, exitCode } = await runFixture(failToConnect("mysql")); + const { stdout, exitCode } = await runFixture(refusedConnectionFixture("mysql")); expect(stdout).toBe( "onclose: ERR_MYSQL_CONNECTION_REFUSED\nuncaught: boom from onclose\nquery rejected: ERR_MYSQL_CONNECTION_REFUSED\n", ); @@ -214,7 +159,8 @@ test.concurrent( // throws), onclose used to be invoked while the adapter was still filling // this.connections, so pool methods that scan that array (flush, isConnected, // close) threw a TypeError on the holes when called from inside the callback. -// The callback is now deferred until the pool is fully constructed. +// The callback is now deferred until the pool is fully constructed. Nothing +// is dialed: password() throws before the connection is created. test.concurrent("postgres: pool calls from onclose are safe when connecting fails synchronously", async () => { const fixture = /* ts */ ` import { SQL } from "bun";