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.

[kanongil]

Yeah, sorry that came out a bit strong. I know you aren't doing that deliberately :-)

I was already looking into making a PR, but my initial approach of allowing 'aborted' to be emitted after a 'close' did not seem viable.

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.

The stream close event says (very reasonably I might add): The 'close' event is emitted when the stream and any of its underlying resources (a file descriptor, for example) have been closed.

I would expect that an active socket still being used is an "underlying resource", and now the event is being emitted before it is actually* closed.

*disregarding that it can be a pipelined connection with a faked close.

[kanongil]

AFAICT the only justification for this new behaviour comes from a desire to refactor the internals in #30625. There does not appear to have been given thought to how the early close effects the ecosystem and APIs (as evidenced by the fact that the APIs still document the old behaviour).

All I'm really advocating for, is that the underlying socket is considered a resource, and thus defers the 'close' event (similar to v14).

[ronag]

What about keep alive connections?

[kanongil]

What about keep alive connections?

They should work as v14. While the socket can continue to live, it is no longer a resource once the response has been delivered.

[kanongil]

FYI, this is probably incompatible with the current autoDestroy functionality, since you will need to distinguish a user call to req.destroy() (which should stop the stream and socket immediately) vs. an autoDestroy req.destroy() (which needs to wait for res to finish before calling the _destroy() cb).

[ronag]

What about keep alive connections?

They should work as v14. While the socket can continue to live, it is no longer a resource once the response has been delivered.

But why would req be different? Once the request body has been delivered the socket is no longer a resource relative to the request, only the response.

[ronag]

I’m sorry. But I don’t see a problem here. The current behavior makes sense to me.

[kanongil]

But why would req be different? Once the request body has been delivered the socket is no longer a resource relative to the request, only the response.

I can see your point, and I think both are valid ways to reason about this – it depends on how holistic your viewpoint is. However, I don't see an advantage to change the behaviour and confuse the ecosystem, as has been done. Especially under the guise of a refactor without full consideration for how this effects related APIs and docs.

[ronag]

I'm not sure it was done under a guise of a refactor. The PR was clearly semver-major. The changes have been out for quite a long time and other than this issue we haven't heard any complaints and we do run CITGM to check for problems in the ecosystem.

Quite the contrary, I believe this reduces confusion in the ecosystem as http streams behave more like normal streams. IncomingMessage has and has had lots of special behaviours since it is just pretending to be a stream.

I do agree the docs need improvement.

[ronag]

the only justification for this new behaviour comes from a desire to refactor the internals in #30625

This is incorrect. The justification is that IncomingMessage should behave as other streams so that you can have some kind of uniform reasoning and assumptions in regards to any stream object.

[kanongil]

I'm not sure it was done under a guise of a refactor. The PR was clearly semver-major. The changes have been out for quite a long time and other than this issue we haven't heard any complaints and we do run CITGM to check for problems in the ecosystem.

Well this probably got a bit confused along the way, especially since it was upgrade to semver-major after the revert, but the initial comment in #33035 (comment) mentions it as a refactor.

Quite the contrary, I believe this reduces confusion in the ecosystem as http streams behave more like normal streams. IncomingMessage has and has had lots of special behaviours since it is just pretending to be a stream.

Yes, this is very good, but I don't see how delaying the 'close' emit, until res is done, conflicts with looking like a normal stream. Or maybe I do? – is it because of tooling that rely on the 'close' event (instead of 'end') to finish consuming the stream data. Eg. this would never work if waiting for res, since node expects req to close immediately:

server.on('response', async (req, res) => {
  try {
    for await (const chunk of req) {
      /* process chunk */
    }

    res.end();  // Never reached!
  }
  catch (err) {
    /* handle it */
  }
});

[kanongil]

I'm not a fan of how the 'close' event has been redefined from a "resource closed" signal, to a "we are done" signal. While there is some overlap in the meanings, it does not always fully correlate and can cause problems when the producer and consumer does not agree on the exact meaning.

Don't get me wrong, node streams sorely needed a "we are done" signal, but it would probably have worked better if it had been signalled using a new event.

[lukiano]

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

Would it be possible to add to Node an AbortController that is triggered when the request is aborted? I'm not sure if this should go in the request, the response, or the socket, but I think it would be useful.

I know that a socket can receive an AbortController to destroy it, but what I'd like if the opposite, one that is triggered by external factors that made the connection to close.

If you think this would be useful I'm happy to try come with the code changes. I know it's not going to be as easy as

req.once('aborted', function() {
  this.requestAbortedController.abort(); // assuming requestAbortedController is a new object in the request.
});

due to the changes mentioned here.