fix: handle cloud-sql-connector unhandled exceptions on close#10555
Conversation
### Description This commit resolves an issue where deploying a Data Connect service backed by Cloud SQL Postgres fails with an unexpected unhandled exception (`CloudSQLConnectorError: The connector was closed`). In `@google-cloud/cloud-sql-connector` version `>= 1.7.0`, the `connector.close()` method destroys underlying sockets by asynchronously emitting an `error` event. Previously, `firebase-tools` closed the `pg.Pool` first, which caused the database client to detach all error listeners from its sockets. Consequently, the later `connector.close()` call caused Node.js to crash due to an unhandled exception on those sockets. To prevent this: 1. Re-ordered cleanup sequences so that `connector.close()` is executed prior to `pool.end()`. 2. Attached an explicit `'error'` handler to `pg.Pool` to ensure any connection teardown errors on idle clients are safely logged and absorbed. 3. Wrapped cleanup functions inside `try/catch` blocks for added defensiveness. Fixes #10549 ### Scenarios Tested - Successfully ran all dataconnect and cloudsql unit tests. - Verified teardown sequence completes gracefully without crash using a local reproduction script. - Verified fix handles both `connect.ts` deployments and `dataconnect-sql-shell.ts` shell exits. ### Sample Commands `firebase deploy --only dataconnect --project <project-id>`
There was a problem hiding this comment.
Code Review
This pull request adds error handling to the PostgreSQL pool and wraps cleanup operations in try/catch blocks to prevent unhandled exceptions during Data Connect deployments. However, the reviewer identified critical issues where resource leaks can occur if pool.connect() fails, as the cleanup functions would not be executed. Additionally, declaring conn after cleanUpFn in connect.ts can trigger linter warnings due to referencing a variable before its declaration.
| pool.on("error", (err) => { | ||
| logger.debug("PostgreSQL pool error:", err); | ||
| }); | ||
| const conn: pg.PoolClient = await pool.connect(); |
There was a problem hiding this comment.
If pool.connect() fails and throws an error, the command action will reject immediately without executing the cleanup block at the end of the function. This results in a resource leak where the connector and pool are left open.
Wrapping pool.connect() in a try/catch block to clean up resources on failure prevents this leak.
| const conn: pg.PoolClient = await pool.connect(); | |
| let conn: pg.PoolClient; | |
| try { | |
| conn = await pool.connect(); | |
| } catch (err) { | |
| try { | |
| connector.close(); | |
| } catch (closeErr) { | |
| logger.debug("Error closing Cloud SQL connector:", closeErr); | |
| } | |
| try { | |
| await pool.end(); | |
| } catch (endErr) { | |
| logger.debug("Error ending pg pool:", endErr); | |
| } | |
| throw err; | |
| } |
joehan
left a comment
There was a problem hiding this comment.
Even better than pinning - thanks!
…thin a try-finally block for robust cleanup
Description
This commit resolves an issue where deploying a Data Connect service backed by Cloud SQL Postgres fails with an unexpected unhandled exception (
CloudSQLConnectorError: The connector was closed).In
@google-cloud/cloud-sql-connectorversion>= 1.7.0, theconnector.close()method destroys underlying sockets by asynchronously emitting anerrorevent. Previously,firebase-toolsclosed thepg.Poolfirst, which caused the database client to detach all error listeners from its sockets. Consequently, the laterconnector.close()call caused Node.js to crash due to an unhandled exception on those sockets.To prevent this:
connector.close()is executed prior topool.end().'error'handler topg.Poolto ensure any connection teardown errors on idle clients are safely logged and absorbed.try/catchblocks for added defensiveness.Fixes #10549
Scenarios Tested
connect.tsdeployments anddataconnect-sql-shell.tsshell exits.Sample Commands
firebase deploy --only dataconnect --project <project-id>Description
Scenarios Tested
Sample Commands