Repository navigation
Missing 'aborted' on IncomingMessage (v16 regression) #40775
Description
Activity
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.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 doesreshas some reliable state to detect this when'close'is emitted?req.abortedwill still befalse.res.on('close', () => { const aborted = !res.writableFinished })
Thanks, though that detection is still a best guess estimate (which might work well enough in practice).
I found another unfortunate fallout from the immediate
reqclose, which is definitely a bug (and regression). A call toIncomingMessage.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.Yea. That’s a documentation bug.
I don't think this is a v16 regression but a deliberate change.
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
IncomingMessageis 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?
@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.
Reacted by Robert NagyYeah, 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.
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).Reacted by Sveinung Tord Røsaker and Zach BjornsonWhat about keep alive connections?
What about keep alive connections?
They should work as v14. While the
socketcan continue to live, it is no longer a resource once the response has been delivered.FYI, this is probably incompatible with the current
autoDestroyfunctionality, since you will need to distinguish a user call toreq.destroy()(which should stop the stream and socket immediately) vs. anautoDestroyreq.destroy()(which needs to wait for res to finish before calling the_destroy()cb).What about keep alive connections?
They should work as v14. While the
socketcan 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.
I’m sorry. But I don’t see a problem here. The current behavior makes sense to me.
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.
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.
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.
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, untilresis 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 forres, since node expectsreqto 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 */ } });
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.
Reacted by Sveinung Tord Røsaker, Dwayne Springfield, Luis Aleman, Stepan Rakitin and LXres.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.
Reacted by Daniel LytkinReacted by Dmitriy
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?
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):
What do you see instead?
Script never exits and misses the last
ABORTEDline from the log: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 onIncomingMessageas 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 thereqobject.I expect the proper solution is to delay the
'close'event untilreshas also finished. This should also fix related issues, like #38924./cc @dnlup