disconnect event does not fire when the route's options.payload.output is data or file

[pgayvallet]

Support plan

• is this issue currently blocking your project? (yes/no): the whole project, no. Blocking some features to be merged, yes.
• is this issue affecting a production system? (yes/no): yes

Context

• node version: 16.13.2
• module version with issue: 20.2.1, probably others
• last module version without issue: unknown
• environment (e.g. node, browser, native): node
• used with (e.g. hapi application, another framework, standalone, ...): hapi application, another framework
• any other relevant information:

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

For some reason, when a route's options.payload.output is set to data or file, canceling the request from the client does not cause the disconnect event (on hapi's request) or the aborted event (on the underlying request.raw.req) to fire. This works fine when output is set to stream, or for non-payload requests such as GET

A complete reproduction scenario:

import { Server } from '@hapi/hapi';
import supertest from 'supertest';

const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));

const deferred = <T = unknown>(): {
  promise: Promise<T>;
  resolve: (value: T) => void;
  reject: (error: any) => void;
} => {
  let resolve: (value: T) => void;
  let reject: (error: any) => void;
  const promise = new Promise<T>((_resolve, _reject) => {
    resolve = _resolve;
    reject = _reject;
  });
  return {
    promise,
    resolve: resolve!,
    reject: reject!,
  };
};

describe('aborted event bug', () => {
  let server: Server;
  let request: supertest.SuperTest<supertest.Test>;

  beforeEach(() => {
    server = new Server({
      host: '127.0.0.1',
      port: 6666,
    });
    request = supertest('http://127.0.0.1:6666');
  });

  afterEach(async () => {
    await server.stop();
  });

  const runTest = async (options: { output: 'stream' | 'data' | 'file' }) => {
    let abortedReceived = false;
    const { promise: requestClosedPromise, resolve: requestClosedResolve } = deferred<void>();

    server.route({
      method: 'POST',
      path: '/test',
      options: {
        payload: {
          output: options.output,
        },
      },
      handler: async (req, h) => {
        req.events.on('disconnect', () => { // same with req.raw.req.on('aborted'
          abortedReceived = true;
        });
        req.raw.res.on('close', () => {
          requestClosedResolve();
        });

        await delay(3000);
        return h.response('foo');
      },
    });

    await server.start();

    const incomingRequest = request.post('/test').send({ hello: 'dolly' }).end();
    setTimeout(() => incomingRequest.abort(), 100);

    await requestClosedPromise;

    expect(abortedReceived).toEqual(true);
  };

  // pass
  it('should work with `options.payload.output=stream`', async () => {
    await runTest({ output: 'stream' });
  });

  // fail
  it('should work with `options.payload.output=data`', async () => {
    await runTest({ output: 'data' });
  });

  // fail
  it('should work with `options.payload.output=file`', async () => {
    await runTest({ output: 'file' });
  });
});

What was the result you got?

 FAIL  aborted_bug.test.ts
  aborted event bug
    ✓ should work with `options.payload.output=stream` (160 ms)
    ✕ should work with `options.payload.output=file` (117 ms)
    ✕ should work with `options.payload.output=data` (113 ms)

What result did you expect?

 PASS  aborted_bug.test.ts
  aborted event bug
    ✓ should work with `options.payload.output=stream` (160 ms)
    ✓ should work with `options.payload.output=file` (117 ms)
    ✓ should work with `options.payload.output=data` (113 ms)

Additional information

If it helps, I somewhat tracked it down to @hapi/subtext, in internals.parse. If you exit early in the function to treat file and data as stream:

--    if (output === 'stream') {
        return source;
--    }

The tests go green.

So it seems likely that something deeper in

await internals.writeFile(req, options, source);

and

const payload = await Wreck.read(source, { timeout: options.timeout, maxBytes: options.maxBytes });

may be causing the problem. Unfortunately after taking a look deeper, I couldn't really understand what could be causing this.

[pgayvallet]

Very similar to #4315, feel free to close if considered duplicate.

[Nargonath]

Thanks for the detailed report. Per the discussion in #4315 and the issue @kanongil opened on the node repo it seems Node will not change their behaviour. I'm not sure what are the required changes but this might need some thought through for sure.

[devinivy]

Indeed, this is the same issue. In node v16 'aborted' is no longer emitted by node's req object once the payload has been delivered. The reason you observe the issue when using file or data is that these ensure the payload has been fully read prior to the handler, which is when the request is aborted in your tests. When you use stream, you are able to abort the request before the payload has been fully read. Unfortunately it has become difficult to properly support disconnect in these cases.

I am closing as a duplicate, but your report is appreciated! Also, I saw that you came-up with a workaround in elastic/kibana#126184. If you'd be open to contributing the approach to hapi, we would love to take a look.

Also found this open issue, which I've subscribed to in case there's any further conversation in node nodejs/node#41117