Store.set throwing in a promise crashes the application in Node v20

niels-van-den-broeck commented 4 months ago

Prerequisites

[X] I have written a descriptive issue title
[X] I have searched existing issues to ensure the regression has not already been reported

Last working version

n.a.

Stopped working in version

n.a.

Node.js version

20.11.1

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

14.3

💥 Regression Report

Having a set function in a custom store which rejects my app to crash in NodeJS v20. In versions before v20 (v18 / v16) this behaviour seemed to work just fine.

The actual application store connects to dynamodb which seems to throw whenever connections are aborted early:

    store: {
      get: (sessionId, callback) => {
        server.DynamoDB.get({ TableName: TABLES.session, Key: { id: sessionId } }).then(
          ({ Item }) => callback(null, Item as any),
        );
      },
      set: (sessionId, session, callback) => {
        server.DynamoDB.put({
          TableName: TABLES.session,
          Item: {
            ...session,
            id: sessionId,
          },
        }).then(() => callback());
      },
      destroy: (sessionId, callback) => {
        server.DynamoDB.delete({ TableName: TABLES.session, Key: { id: sessionId } }).then(() =>
          callback(),
        );
      },
    },

Steps to Reproduce

I have created a repo with a minimal reproduction example.

The important file is src/server.ts which sets up the session middleware. To emulate dynamodb failing I have a let which I set to false, making the set function reject when a connection is interrupted.

Steps:

Checkout repo
move into repo
nvm use
npm i
npm run start
open separate terminal tab
move into repo
chmod +x ./scripts/test-load.sh && ./scripts/test-load.sh
CTRL + C out of the script
The application on the first tab crashed

If you change the nodejs version to v18.x (18.18.0 in my case) in the .nvmrc, reinstall, and retry the steps above you will see that the application won't crash.

Expected Behavior

The application does not crash.

I am not sure if this is intended behaviour, but it seemed off since it changes according to your nodejs version.

niels-van-den-broeck commented 4 months ago

Also please keep in mind that the only purpose of the code in the attached repo is to reproduce the issue we were facing in proprietary code.

It does not produce the same error, but has the same behaviour as we have in our production code.

The error we received in our application was the following:

Error: Cannot write headers after they are sent to the client
      at ServerResponse.writeHead (node:_http_server:345:11)
      at onSendEnd (/node_modules/fastify/lib/reply.js:571:9)
      at wrapOnSendEnd (/node_modules/fastify/lib/reply.js:537:5)
      at next (/node_modules/fastify/lib/hooks.js:218:7)
      at /node_modules/@fastify/session/lib/fastifySession.js:190:9
      at /node_modules/@fastify/session/lib/session.js:171:9
      at /src/plugins/sessions.ts:39:23
      at processTicksAndRejections (node:internal/process/task_queues:95:5) {
    code: 'ERR_HTTP_HEADERS_SENT'
  }

Eomm commented 4 months ago

Here, could you try this: https://github.com/niels-van-den-broeck/FastifyV20Repro/blob/a61a4aba1f4ac64e0edfb92231a6dfb1aaef2b67/src/server.ts#L46-L47

    set: (sessionId, session, callback) => {
      new Promise<void>((resolve, reject) => {
        setTimeout(() => {
-          if (cancelled) reject();
+          if (cancelled) { reject(); return };
          resolve();
        }, 150);
      }).then(() => {
        // eslint-disable-next-line @typescript-eslint/ban-ts-comment
        // @ts-ignore
        sessions.set(sessionId, {...session, id: sessionId });
        callback();
      });
    },

mrbrianevans commented 4 months ago

I'm getting this error after upgrading from Node 19 to Node 22:

/filing-deadlines/node_modules/.pnpm/standard-as-callback@2.1.0/node_modules/standard-as-callback/built/index.js:6
        throw e;
        ^

Error [ERR_HTTP_HEADERS_SENT]: Cannot write headers after they are sent to the client
    at ServerResponse.writeHead (node:_http_server:345:11)
    at safeWriteHead (/filing-deadlines/node_modules/.pnpm/fastify@4.27.0/node_modules/fastify/lib/reply.js:572:9)
    at onSendEnd (/filing-deadlines/node_modules/.pnpm/fastify@4.27.0/node_modules/fastify/lib/reply.js:621:5)
    at wrapOnSendEnd (/filing-deadlines/node_modules/.pnpm/fastify@4.27.0/node_modules/fastify/lib/reply.js:565:5)
    at next (/filing-deadlines/node_modules/.pnpm/fastify@4.27.0/node_modules/fastify/lib/hooks.js:289:7)
    at /filing-deadlines/node_modules/.pnpm/@fastify+session@10.8.0/node_modules/@fastify/session/lib/fastifySession.js:200:9
    at /filing-deadlines/node_modules/.pnpm/@fastify+session@10.8.0/node_modules/@fastify/session/lib/session.js:180:11
    at tryCatcher (/filing-deadlines/node_modules/.pnpm/standard-as-callback@2.1.0/node_modules/standard-as-callback/built/utils.js:12:23)
    at /filing-deadlines/node_modules/.pnpm/standard-as-callback@2.1.0/node_modules/standard-as-callback/built/index.js:22:53
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  code: 'ERR_HTTP_HEADERS_SENT'
}

Node.js v22.2.0

I can't tell from that stack trace where the error originates. Is this an issue with the fastify session package, or an issue with my usage of it? It worked fine with previous versions of node.

Much appreciated

niels-van-den-broeck commented 4 months ago

Here, could you try this: https://github.com/niels-van-den-broeck/FastifyV20Repro/blob/a61a4aba1f4ac64e0edfb92231a6dfb1aaef2b67/src/server.ts#L46-L47

    set: (sessionId, session, callback) => {
      new Promise<void>((resolve, reject) => {
        setTimeout(() => {
-          if (cancelled) reject();
+          if (cancelled) { reject(); return };
          resolve();
        }, 150);
      }).then(() => {
        // eslint-disable-next-line @typescript-eslint/ban-ts-comment
        // @ts-ignore
        sessions.set(sessionId, {...session, id: sessionId });
        callback();
      });
    },

I have a solution, which is just catching the error DynamoDB throws in our application and calling the callback. I was just trying to point out the change in behaviour between Node versions.

mcollina commented 4 months ago

In your original examples you were not handling errors, I'm actually surprised it even worked to begin with :D.

fastify / session