Missing 'aborted' on IncomingMessage (v16 regression)

[kanongil]

Version

v16.13.0 / v17.1.0

Platform

Darwin Silmaril.local 21.1.0 Darwin Kernel Version 21.1.0: Wed Oct 13 17:33:24 PDT 2021; root:xnu-8019.41.5~1/RELEASE_ARM64_T8101 arm64

Subsystem

http

What steps will reproduce the bug?

'use strict';

const http = require('http');

let clientReq;
const server = http.createServer(function (req, res) {
    req.on('aborted', function () {
        console.log('ABORTED');
        server.close();
    });

    req.on('end', () => {
        console.log('DESTROY');
        clientReq.destroy();
    });

    req.resume(); // read all client data
});

server.listen(0, () => {
    clientReq = http.request({
        method: 'POST',
        port: server.address().port,
        headers: { connection: 'keep-alive' }
    }, (res) => {
    });

    clientReq.on('error', (err) => console.log('CLIENT ERROR', err));

    clientReq.end();
});

How often does it reproduce? Is there a required condition?

100%

What is the expected behavior?

Script exits and logs the following (node v14.18.1):

DESTROY
CLIENT ERROR Error: socket hang up
    at connResetException (internal/errors.js:639:14)
    at Socket.socketCloseListener (_http_client.js:449:25)
    at Socket.emit (events.js:412:35)
    at TCP.<anonymous> (net.js:686:12) {
  code: 'ECONNRESET'
}
ABORTED

What do you see instead?

Script never exits and misses the last ABORTED line from the log:

DESTROY
CLIENT ERROR Error: socket hang up
    at connResetException (node:internal/errors:691:14)
    at Socket.socketCloseListener (node:_http_client:420:25)
    at Socket.emit (node:events:402:35)
    at TCP.<anonymous> (node:net:687:12) {
  code: 'ECONNRESET'
}

Additional information

This seems to be a fallout for #33035, which had to be reverted in the v15 release line, but now included in v16+. I discovered it while investigating hapijs/hapi#4315.

Essentially 'close' is now emitted on IncomingMessage as soon as all data has been consumed, regardless of whether the underlying stream is still open (ie. the response is still being delivered). Thus node is clearly violating its own streaming API, since the underlying socket is still very much active, and will occasionally need to signal 'aborted' on the req object.

I expect the proper solution is to delay the 'close' event until res has also finished. This should also fix related issues, like #38924.

/cc @dnlup

[ronag]

#33035 is semver-major and I think this is the correct behaviour? @nodejs/http

The incoming message should not be aborted if it has completed...?

[ronag]

@mcollina wdyt?

[kanongil]

The incoming message should not be aborted if it has completed...?

That could indeed make sense, but afaik there is no alternative way to detect that the underlying socket closed unexpectedly during response transmission. We will need a replacement API.

Actually, for a possibly semver-major change, res could emit an 'error' rather than just 'close' when the client aborts.

[kanongil]

Actually, we could detect client aborts (after the payload has been delivered) by listening to both res 'close', 'finish' and 'error', and infer that it must have been caused by a client abort if the 'close' is emitted without a previous 'finish' or 'error' emit. Or does res has some reliable state to detect this when 'close' is emitted? req.aborted will still be false.

[ronag]

res.on('close', () => {
  const aborted = !res.writableFinished
})

[kanongil]

Thanks, though that detection is still a best guess estimate (which might work well enough in practice).

[kanongil]

I found another unfortunate fallout from the immediate req close, which is definitely a bug (and regression). A call to IncomingMessage.destroy() now does nothing when called after the payload has been received. This clearly does not match the current docs:

Calls destroy() on the socket that received the IncomingMessage.

[ronag]

Yea. That’s a documentation bug.

[mcollina]

I don't think this is a v16 regression but a deliberate change.

[kanongil]

Here is another documentation bug for the 'close' event then: https://nodejs.org/api/http.html#event-close_2

Indicates that the underlying connection was closed

Do you really expect devs to disregard the documentation and existing behaviour for a some unknown concept on when an IncomingMessage is closed, that goes contrary to the existing definition in the stream module?

@mcollina A deliberate change to break compatibility and make node more confusing to use?

[mcollina]

@kanongil It looks like you have strong opinions.. so would you like to help? We are always looking for more people to review and fix bugs in HTTP and Streams.

PRs are always welcomed.

---

that goes contrary to the existing definition in the stream module?

Please articulate. My understanding is that the change resolve some long-standing problems of IncomingMessage.

A deliberate change to break compatibility and make node more confusing to use?

I do not agree with that statement. I think those changes makes it less confusing to use from a Stream perspective.