diff --git a/src/js/internal/sql/mysql.ts b/src/js/internal/sql/mysql.ts index 4786bfc8457..62594194044 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 655f2509f18..d778b6e0687 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 00000000000..35d91b09315 --- /dev/null +++ b/test/js/sql/sql-onconnect-onclose-throw.test.ts @@ -0,0 +1,198 @@ +// 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 +// +// 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, describeWithContainer, isDockerEnabled, tempDir } from "harness"; + +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 }, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; +} + +// 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); +`; +} + +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); + }); + }); + + 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, 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() { + 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 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 ( + 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 onclose callback still rejects pending queries when the connection is refused", + async () => { + 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", + ); + 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(refusedConnectionFixture("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. 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"; +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); +});