hapijs / hapi

The Simple, Secure Framework Developers Trust
https://hapi.dev
Other
14.6k stars 1.34k forks source link

request disconnect event does not trigger for certain HTTP methods under node v16 #4315

Open FaHeymann opened 2 years ago

FaHeymann commented 2 years ago

Support plan

Context

What are you trying to achieve or the steps to reproduce?

It seems like the disconnect event of requests does only fire for GET requests but not for other HTTP methods starting from nodeJS v16. See also the test script below. With node v15 it fires for all of them. In both versions the finish event also seems to not trigger for GET. I don't know why there are differences in the bahaviour between http methods in the first place.

const Hapi = require('@hapi/hapi');
const net = require('net');
const { setTimeout } = require('timers/promises');

const check = async (method) => {
  console.log(method)
  const server = Hapi.server({
    port: 8000,
    host: '0.0.0.0',
  })

  server.ext('onRequest', (request, h) => {
    request.events.on('disconnect', () => { console.log('disconnect') });
    request.events.on('finish', () => { console.log('finish') });

    return h.continue;
  })

  server.route({
    method: method,
    path: '/test',
    handler: async (request, h) => {
      await setTimeout(1000);
      return h.response();
    },
  });

  await server.start();

  const socket = new net.Socket();
  socket.connect({
    host: server.info.host,
    port: server.info.port,
  });

  await new Promise ((resolve) => {
    socket.on('connect', function () {
      socket.write(`${method} /test HTTP/1.1\r\n\r\n`, async () => {
        socket.destroy();
        await setTimeout(100);
        await server.stop();
        resolve();
      });
    });
  });
  console.log('');
}

(async () => {
  await check('GET');
  await check('POST');
  await check('PUT');
  await check('DELETE');
})();

What was the result you got?

Node 16:

GET
disconnect

POST
finish

PUT
finish

DELETE
finish

Node 15:

GET
disconnect

POST
finish
disconnect

PUT
finish
disconnect

DELETE
finish
disconnect

What result did you expect?

I expected the disconnect event to fire in all cases.

kanongil commented 2 years ago

Thanks for the report. Node v16 has a change that causes issues for requests with payloads. We have worked to support it fully, but it seems that it is still causing trouble.

kanongil commented 2 years ago

Looking at the docs, hapi does not guarantee that 'disconnect' is called. In fact the v15 behaviour could be considered a bug:

'disconnect' - emitted when a request errors or aborts unexpectedly.

Maybe you are using the event for something that it is not intended to handle?

Edit: I misread your code. A 'disconnect' event should definitely happen.

kanongil commented 2 years ago

Hmm, this could be a node bug. It seems that http.IncomingMessage no longer emits 'aborted' events once the payload has been delivered. It also seems that node will now deliver 'error' emits (with Error: aborted) after the 'close' event!

It seems node wants to deprecate 'aborted'.

Working on a patch that works around this issue by not relying on the 'aborted' event (or property).

kanongil commented 2 years ago

There does not appear to be a non-hacky way to work around this issue with node v16+. I have reported the underlying issue here: https://github.com/nodejs/node/issues/40775.

ronag commented 2 years ago

It also seems that node will now deliver 'error' emits (with Error: aborted) after the 'close' event!

Do you have a repro for that? It shouldn't happen.

kanongil commented 2 years ago

It also seems that node will now deliver 'error' emits (with Error: aborted) after the 'close' event!

Do you have a repro for that? It shouldn't happen.

I don't think it actually does. I likely misread the initial testing I was doing.

joshkel commented 2 months ago

In my local testing, an aborted POST results in Hapi logging a request error close event, so it seems that it is detecting the disconnect at some level?

If so, then would it help to update lib/request.js's internals.event to emit a disconnect event on close as well as abort?

(I could easily be missing or misunderstanding something here - I'm unclear on the difference between request error close and request error abort.)