GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http: add test case for req-res close ordering by dnlup · Pull Request #36645 · nodejs/node

Conversation

@dnlup

@dnlup dnlup commented Dec 27, 2020

Copy link
Copy Markdown
Contributor

Since the PR #33035 has changed the ordering of the close event, add a test case.
IncomingMessage will emit close before the response is sent in case the server is consuming data from it.
See comment #33035 (comment)

As @kanongil pointed out, IncomingMessage is emitting close before the response when the server attaches a handler to the request data event. Otherwise, the order is the same as it was before the change in the linked PR.

The test should catch eventual regressions, but let's also use this as a point for discussing this behavior further, if needed.

@ronag @mcollina

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Dec 27, 2020
Trott
Trott reviewed Dec 27, 2020
View reviewed changes
Comment thread test/parallel/test-http-req-res-close.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Dec 27, 2020
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if all the added tests pass reliably.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ronag
ronag approved these changes Dec 27, 2020
View reviewed changes
@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. author ready PRs with CI started, the required approvals, and no outstanding review comments. labels Dec 27, 2020
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 27, 2020
@nodejs-github-bot

nodejs-github-bot commented Dec 27, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35099/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
ronag requested a review from mcollina December 27, 2020 19:17
mcollina
mcollina approved these changes Dec 27, 2020
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@devinivy devinivy mentioned this pull request Dec 28, 2020
Crashes upon request on Node.js v15.5.0 hapijs/hapi#4208
Closed
@dnlup
dnlup force-pushed the fix/regression_test_http_incoming branch from 37662dd to b415015 Compare December 28, 2020 09:43
@dnlup

dnlup commented Dec 28, 2020

Copy link
Copy Markdown
Contributor Author

LGTM if all the added tests pass reliably.

Thinking out loud about the tests, I am wondering if this behavior could cause problems. In the first two cases res emits close after req, while in the last one is the other way around, as already pointed out. In the previous implementation, it seems that the order was always the same.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dnlup

dnlup commented Dec 28, 2020

Copy link
Copy Markdown
Contributor Author

I added a few more comments.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dnlup
dnlup force-pushed the fix/regression_test_http_incoming branch from b415015 to 5b2c944 Compare December 28, 2020 10:22
@mcollina

mcollina commented Dec 28, 2020

Copy link
Copy Markdown
Member

I still think this behavior is ok. I would like to know a few arguments on why this is bad (minus it was a breaking change as a minor).
dnlup reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dnlup

dnlup commented Dec 28, 2020

Copy link
Copy Markdown
Contributor Author

I still think this behavior is ok. I would like to know a few arguments on why this is bad (minus it was a breaking change as a minor).

My concern is with framework/libraries that might expect a strict ordering, always. I don't know if that is a good practice or not. This behavior broke hapi, but I tested fastify out of curiosity, and it didn't suffer the same issue. TLDR, I hope this doesn't complicate things too much in codebases that broke because of this change (i.e., when a fix would have to be implemented on their side). That's all I can say. I don't have a deep knowledge of hapi internals to expand more on that atm. Maybe I just got a little worried after yesterday.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Dec 28, 2020

Copy link
Copy Markdown
Member

@cjihrig wdyt? I don't really understand how Hapi relies on this specific ordering of things.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kanongil

kanongil commented Dec 28, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I did the analysis for hapi and can elaborate.

Hapi adds a 'close' listener to the http.IncomingRequest and will terminate the response processing when this is emitted. This broke since req would suddenly close as soon as all incoming data was processed.

The specific ordering of the req & res 'close' emits are irrelevant, since hapi only listens to the req event. Other frameworks are not affected because they don't take the same holistic approach that hapi does.

Note that for hapi I tried to change this to listen on the 'close' event of the http.ServerResponse instead. This fixed the issue, except for an upload socket close test that is most likely wrong (it uses synthesized req / res and events).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dnlup

dnlup commented Dec 28, 2020

Copy link
Copy Markdown
Contributor Author

Thank you @kanongil for the analysis. It was very helpful.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Dec 28, 2020

Copy link
Copy Markdown
Member

@kanongil could you confirm that there is no significant issue with that change shipping in Node.js v16?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 3, 2021
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 3, 2021
@nodejs-github-bot

nodejs-github-bot commented Jan 3, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35228/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dnlup

dnlup commented Jan 4, 2021

Copy link
Copy Markdown
Contributor Author

I can't see on https://ci.nodejs.org/ why those 3 jobs are failing, unfortunately.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 4, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35251/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina added the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 5, 2021
@github-actions github-actions Bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Jan 5, 2021
@github-actions

github-actions Bot commented Jan 5, 2021

Copy link
Copy Markdown
Contributor
Commit Queue failed
- Loading data for nodejs/node/pull/36645
✔  Done loading data for nodejs/node/pull/36645
----------------------------------- PR info ------------------------------------
Title      http: add test case for req-res close ordering (#36645)
Author     Daniele Belardi  (@dnlup)
Branch     dnlup:fix/regression_test_http_incoming -> nodejs:master
Labels     author ready, test
Commits    1
 - http: add test case for req-res close ordering
Committers 1
 - Daniele Belardi 
PR-URL: https://github.com/nodejs/node/pull/36645
Reviewed-By: Rich Trott 
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36645
Reviewed-By: Rich Trott 
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - http: add test case for req-res close ordering
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2021-01-04T21:19:02Z: https://ci.nodejs.org/job/node-test-pull-request/35251/
- Querying data for job/node-test-pull-request/35251/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sun, 27 Dec 2020 18:06:44 GMT
   ✔  Approvals: 3
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36645#pullrequestreview-558953069
   ✔  - Robert Nagy (@ronag): https://github.com/nodejs/node/pull/36645#pullrequestreview-558953108
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/36645#pullrequestreview-558962536
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/463731724
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Jan 5, 2021
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina mcollina added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Jan 5, 2021
@github-actions github-actions Bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 5, 2021
@dnlup @aduh95
http: add test case for req-res close ordering
ddac3bd 
Since 55e83cb
has changed the ordering of the close event, add a test case.
IncomingMessage will emit close before the response is sent in case the
server is consuming data from it.

Refs: nodejs#33035 (comment)

PR-URL: nodejs#36645
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@aduh95
aduh95 force-pushed the fix/regression_test_http_incoming branch from 5b2c944 to ddac3bd Compare January 6, 2021 10:27
@aduh95

aduh95 commented Jan 6, 2021

Copy link
Copy Markdown
Contributor

Landed in ddac3bd
dnlup reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit ddac3bd into nodejs:master Jan 6, 2021
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@dnlup @danielleadams
http: add test case for req-res close ordering
dfc962f 
Since 55e83cb
has changed the ordering of the close event, add a test case.
IncomingMessage will emit close before the response is sent in case the
server is consuming data from it.

Refs: #33035 (comment)

PR-URL: #36645
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
v15.6.0 proposal #36889
Merged
@targos targos added the backport-blocked-v14.x label May 25, 2021
@coderabbitai coderabbitai Bot mentioned this pull request Jul 12, 2026
feat(webui): complete zero-secret acceptance GhostXia/AIRP#123
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Jul 24, 2026
fix(import): add request timeout and cancellation to PDF import (closes #15) harsharajkumar-273/Proofdesk#112
Merged
This was referenced Aug 4, 2026
fix(chat): add heartbeat pings and abort streams on client disconnect rwr-infra/rwr-data-agent#8
Merged
feat(cli): add experimental voice gateway NVIDIA/NemoClaw#8429
Merged
This was referenced Aug 22, 2026
Cap POST bodies and apply nginx hardening from Actions bcProFoundation/wlotus#221
Merged
Add screenshot fixtures and capture tooling for documentation KEGustafsson/signalk-portainer#33
Merged
@qodo-code-review qodo-code-review Bot mentioned this pull request Aug 27, 2026
Feat/monorepo cockpit daniyalahmed21/deadman#7
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@Trott Trott Trott approved these changes
@ronag ronag ronag approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@dnlup @nodejs-github-bot @mcollina @kanongil @aduh95 @Trott @ronag @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.