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

[FaHeymann]

Support plan

• is this issue currently blocking your project? (yes/no): no
• is this issue affecting a production system? (yes/no): no

Context

• node version: v16.12.0 / v15.14.0
• module version with issue: 20.2.1
• last module version without issue: n/a
• environment (e.g. node, browser, native): node
• used with (e.g. hapi application, another framework, standalone, ...): n/a
• any other relevant information:

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]

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]

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]

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]

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: nodejs/node#40775.

[ronag]

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]

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]

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.)