Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AbortSignal to NodePgPreparedQuery #3954

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sybereal
Copy link

This signal is aborted from the NodePgSession object when an error event occurs on the underlying node-postgres client object. By default, there is no handler for such errors, meaning they will crash the Node.js process.
The AbortSignal acts as a "buffer" for the error event, storing it and turning it into a promise rejection that will then travel up the call stack, allowing the code that invoked Drizzle to decide what to do with it.

Closes #3908.

This is a minimal reproducer for the error case this addresses:

import { setTimeout } from "node:timers/promises";

import { sql } from "drizzle-orm";
import { drizzle } from "drizzle-orm/node-postgres";

const db = drizzle("postgres://postgres:asdf@localhost:5432/postgres");

try {
  await db.transaction(async (tx) => {
    const {
      rows: [{ pid }],
    } = await tx.execute("select pg_backend_pid() as pid");

    Promise.resolve(db.execute(sql`select pg_terminate_backend(${pid})`));

    await setTimeout(5000);
  });
} finally {
  console.log("Finally");
}

Before this patch, this results in an uncaught error terminating the process. Notably, the print in the finally block is never executed.

node:events:502
      throw er; // Unhandled 'error' event
      ^

error: terminating connection due to administrator command
    at Parser.parseErrorMessage (/home/user/Scratchpad/drizzle-repro-pgerror/node_modules/.pnpm/[email protected]/node_modules/pg-protocol/src/parser.ts:368:69)
    at Parser.handlePacket (/home/user/Scratchpad/drizzle-repro-pgerror/node_modules/.pnpm/[email protected]/node_modules/pg-protocol/src/parser.ts:187:21)
    at Parser.parse (/home/user/Scratchpad/drizzle-repro-pgerror/node_modules/.pnpm/[email protected]/node_modules/pg-protocol/src/parser.ts:102:30)
    at Socket.<anonymous> (/home/user/Scratchpad/drizzle-repro-pgerror/node_modules/.pnpm/[email protected]/node_modules/pg-protocol/src/index.ts:7:48)
    at Socket.emit (node:events:524:28)
    at addChunk (node:internal/streams/readable:561:12)
    at readableAddChunkPushByteMode (node:internal/streams/readable:512:3)
    at Readable.push (node:internal/streams/readable:392:5)
    at TCP.onStreamRead (node:internal/stream_base_commons:189:23)
Emitted 'error' event on Client instance at:
    at Client._handleErrorEvent (/home/user/Scratchpad/drizzle-repro-pgerror/node_modules/.pnpm/[email protected]/node_modules/pg/lib/client.js:340:10)
    at Client._handleErrorMessage (/home/user/Scratchpad/drizzle-repro-pgerror/node_modules/.pnpm/[email protected]/node_modules/pg/lib/client.js:351:12)
    at Connection.emit (node:events:524:28)
    at /home/user/Scratchpad/drizzle-repro-pgerror/node_modules/.pnpm/[email protected]/node_modules/pg/lib/connection.js:116:12
    at Parser.parse (/home/user/Scratchpad/drizzle-repro-pgerror/node_modules/.pnpm/[email protected]/node_modules/pg-protocol/src/parser.ts:103:9)
    at Socket.<anonymous> (/home/user/Scratchpad/drizzle-repro-pgerror/node_modules/.pnpm/[email protected]/node_modules/pg-protocol/src/index.ts:7:48)
    [... lines matching original stack trace ...]
    at Readable.push (node:internal/streams/readable:392:5) {
  length: 116,
  severity: 'FATAL',
  code: '57P01',
  detail: undefined,
  hint: undefined,
  position: undefined,
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  schema: undefined,
  table: undefined,
  column: undefined,
  dataType: undefined,
  constraint: undefined,
  file: 'postgres.c',
  line: '3246',
  routine: 'ProcessInterrupts'
}

Node.js v22.13.0

With this patch, such errors are now caught by the session and rethrown on next attempted use. As you can see, the finally block executes as expected; the process doesn't just crash. Also, the stack trace, while not complete due to a v8 bug with custom thenables, now clearly leads into Drizzle code at the location that tried to use the connection next.

Finally
/home/user/Code/drizzle-team/drizzle-orm/drizzle-orm/src/node-postgres/session.ts:91
                                throw new Error('PostgreSQL connection experienced an error or has been closed', {cause: e});
                                      ^


Error: PostgreSQL connection experienced an error or has been closed
    at <anonymous> (/home/user/Code/drizzle-team/drizzle-orm/drizzle-orm/src/node-postgres/session.ts:91:11)
    at Object.startActiveSpan (/home/user/Code/drizzle-team/drizzle-orm/drizzle-orm/src/tracing.ts:27:11)
    at NodePgPreparedQuery.execute (/home/user/Code/drizzle-team/drizzle-orm/drizzle-orm/src/node-postgres/session.ts:87:17)
    at QueryPromise.execute (/home/user/Code/drizzle-team/drizzle-orm/drizzle-orm/src/pg-core/db.ts:616:19)
    at QueryPromise.then (/home/user/Code/drizzle-team/drizzle-orm/drizzle-orm/src/query-promise.ts:32:15) {
  [cause]: error: terminating connection due to administrator command
      at Parser.parseErrorMessage (/home/user/Code/drizzle-team/drizzle-orm/node_modules/.pnpm/[email protected]/node_modules/pg-protocol/src/parser.ts:369:69)
      at Parser.handlePacket (/home/user/Code/drizzle-team/drizzle-orm/node_modules/.pnpm/[email protected]/node_modules/pg-protocol/src/parser.ts:188:21)
      at Parser.parse (/home/user/Code/drizzle-team/drizzle-orm/node_modules/.pnpm/[email protected]/node_modules/pg-protocol/src/parser.ts:103:30)
      at Socket.<anonymous> (/home/user/Code/drizzle-team/drizzle-orm/node_modules/.pnpm/[email protected]/node_modules/pg-protocol/src/index.ts:7:48)
      at Socket.emit (node:events:524:28)
      at addChunk (node:internal/streams/readable:561:12)
      at readableAddChunkPushByteMode (node:internal/streams/readable:512:3)
      at Readable.push (node:internal/streams/readable:392:5)
      at TCP.onStreamRead (node:internal/stream_base_commons:189:23) {
    length: 116,
    severity: 'FATAL',
    code: '57P01',
    detail: undefined,
    hint: undefined,
    position: undefined,
    internalPosition: undefined,
    internalQuery: undefined,
    where: undefined,
    schema: undefined,
    table: undefined,
    column: undefined,
    dataType: undefined,
    constraint: undefined,
    file: 'postgres.c',
    line: '3246',
    routine: 'ProcessInterrupts'
  }
}

Node.js v22.13.0

Due to the aforementioned stack trace issue, I wasn't quite sure where to place the re-throw in NodePgPreparedQuery.execute. For now, I placed it at the start of the method, but I was wondering if it might be better to put it after the logQuery invocation or even inside the drizzle.driver.execute spans, to ensure observability tools can fill the gap.

This signal is aborted from the NodePgSession object when an error event
occurs on the underlying node-postgres client object. By default, there
is no handler for such errors, meaning they will crash the Node.js
process.
The AbortSignal acts as a "buffer" for the error event, storing it and
turning it into a promise rejection that will then travel up the call
stack, allowing the code that invoked Drizzle to decide what to do with
it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: Unhandled error event on pg.Client crashes process
1 participant