fix: replace deprecated 'aborted' event with 'close' on IncomingMessage#4579
fix: replace deprecated 'aborted' event with 'close' on IncomingMessage#4579sapirbaruch wants to merge 4 commits into
Conversation
|
@sapirbaruch Can you please add tests for this? |
Adds a test verifying that request.active() returns false when the client closes the connection before a response is sent. The test uses Net.connect so it works regardless of hostname resolution, and polls active() directly without relying on the 'disconnect' event. Covers the Node.js v24+ path where IncomingMessage 'aborted' is gone.
|
Added a regression test in The test uses |
|
@Marsup can we get an approval to run the CI on this? |
|
As expected, it fails on "old" node.js versions. I think we need both subscriptions to maintain compatibility. EDIT: actually it even fails on latest, so I'm not sure it's quite right. |
The 'aborted' event on IncomingMessage was deprecated in Node.js v17 and removed in Node.js v24. Restore compatibility by listening to both 'aborted' (for older Node) and the 'close' event on the response (for Node v24+ where 'aborted' no longer fires). A 'close' on the response before writableEnded indicates the client disconnected mid-request and is treated as an abort. @hapi/shot inject requests are excluded via the req._shot sentinel so that simulate.close scenarios are not misclassified as client aborts.
|
Howdy @damusix , looks like the tests are failing due to line coverage. |
| // inject requests (identified by req._shot) because shot fires req.emit('close') for its | ||
| // simulate.close scenario but that should not be treated as a client abort. | ||
| if (event === 'abort' || | ||
| (event === 'close' && !request.raw.res.writableEnded && !request.raw.req._shot)) { |
There was a problem hiding this comment.
We just published a new version of shot that might change the behaviour. Regardless, we should probably not specialcase shot here, but rather revise the tests. Treating it as an aborted request seems quite appropriate.
I guess add more tests 😉 FYI, you can do |
…tainer guidance Per feedback on hapijs#4579: a premature socket close (simulate.close or a real TCP disconnect on Node.js v24+) is correctly treated as a client abort. Remove the req._shot guard that was suppressing abort detection for shot inject requests and update the test to expect 499 instead of 500. The false branch of the final condition is structurally unreachable — after all early returns, only 'abort' and 'close' events reach that point — so a $lab:coverage:off$ annotation is used for the one unavoidable uncovered branch.
|
Hey @kanongil — just wanted to flag that the coverage issues you mentioned on May 27 were addressed in the May 28 push. All CI checks are green across Ubuntu and Windows (Node 14–latest). Would appreciate a second look when you get a chance! |
kanongil
left a comment
kanongil
left a comment
There was a problem hiding this comment.
Wait, what is going on here? The added test doesn't fail for me when using the current logic (on any node release).
Also the claim that "IncomingMessage 'aborted' event was removed in Node.js v24." is patently false, and a pure hallucination.
5 participants
The
'aborted'event onhttp.IncomingMessagewas deprecated in Node.js v17 and no longer fires in v24+. As a result,Request.active()always returnstruebecause_eventContext.requestis never set tonullon client disconnects.The fix replaces
req.on('aborted', ...)withreq.on('close', ...)and guards the abort logic with ares.writableEndedcheck — the same guard used by the existingres.on('close')handler — to distinguish a premature client disconnect from a normal connection teardown.Fixes #4575.