http: add test case for req-res close ordering #36645

dnlup · 2020-12-27T18:06:44Z

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

test/parallel/test-http-req-res-close.js

Trott

LGTM if all the added tests pass reliably.

nodejs-github-bot · 2020-12-27T19:14:38Z

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

mcollina

lgtm

dnlup · 2020-12-28T09:49:33Z

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.

dnlup · 2020-12-28T10:13:27Z

I added a few more comments.

mcollina · 2020-12-28T11:18:21Z

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 · 2020-12-28T13:40:20Z

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.

mcollina · 2020-12-28T15:34:29Z

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

kanongil · 2020-12-28T16:52:55Z

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).

dnlup · 2020-12-28T17:42:12Z

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

mcollina · 2020-12-28T17:55:41Z

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

nodejs-github-bot · 2021-01-03T18:04:20Z

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

dnlup · 2021-01-04T07:34:00Z

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

nodejs-github-bot · 2021-01-04T21:19:02Z

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

github-actions · 2021-01-05T15:38:45Z

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

mcollina

lgtm

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 <rtrott@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

aduh95 · 2021-01-06T10:28:05Z

Landed in 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: #33035 (comment) PR-URL: #36645 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 27, 2020

Trott reviewed Dec 27, 2020

View reviewed changes

test/parallel/test-http-req-res-close.js Outdated Show resolved Hide resolved

Trott approved these changes Dec 27, 2020

View reviewed changes

ronag approved these changes Dec 27, 2020

View reviewed changes

ronag added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 27, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2020

ronag requested a review from mcollina December 27, 2020 19:17

mcollina approved these changes Dec 27, 2020

View reviewed changes

devinivy mentioned this pull request Dec 28, 2020

Crashes upon request on Node.js v15.5.0 hapijs/hapi#4208

Closed

dnlup force-pushed the fix/regression_test_http_incoming branch from 37662dd to b415015 Compare December 28, 2020 09:43

dnlup force-pushed the fix/regression_test_http_incoming branch from b415015 to 5b2c944 Compare December 28, 2020 10:22

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2021

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2021

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 5, 2021

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 5, 2021

mcollina approved these changes Jan 5, 2021

View reviewed changes

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 5, 2021

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 5, 2021

aduh95 force-pushed the fix/regression_test_http_incoming branch from 5b2c944 to ddac3bd Compare January 6, 2021 10:27

aduh95 merged commit ddac3bd into nodejs:master Jan 6, 2021

danielleadams mentioned this pull request Jan 12, 2021

v15.6.0 proposal #36889

Merged

targos added the backport-blocked-v14.x label May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: add test case for req-res close ordering #36645

http: add test case for req-res close ordering #36645

dnlup commented Dec 27, 2020

Trott left a comment

nodejs-github-bot commented Dec 27, 2020

mcollina left a comment

dnlup commented Dec 28, 2020

dnlup commented Dec 28, 2020

mcollina commented Dec 28, 2020

dnlup commented Dec 28, 2020

mcollina commented Dec 28, 2020

kanongil commented Dec 28, 2020 •

edited

Loading

dnlup commented Dec 28, 2020

mcollina commented Dec 28, 2020

nodejs-github-bot commented Jan 3, 2021

dnlup commented Jan 4, 2021

nodejs-github-bot commented Jan 4, 2021

github-actions bot commented Jan 5, 2021

mcollina left a comment

aduh95 commented Jan 6, 2021

http: add test case for req-res close ordering #36645

http: add test case for req-res close ordering #36645

Conversation

dnlup commented Dec 27, 2020

Checklist

Trott left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 27, 2020

mcollina left a comment

Choose a reason for hiding this comment

dnlup commented Dec 28, 2020

dnlup commented Dec 28, 2020

mcollina commented Dec 28, 2020

dnlup commented Dec 28, 2020

mcollina commented Dec 28, 2020

kanongil commented Dec 28, 2020 • edited Loading

dnlup commented Dec 28, 2020

mcollina commented Dec 28, 2020

nodejs-github-bot commented Jan 3, 2021

dnlup commented Jan 4, 2021

nodejs-github-bot commented Jan 4, 2021

github-actions bot commented Jan 5, 2021

mcollina left a comment

Choose a reason for hiding this comment

aduh95 commented Jan 6, 2021

kanongil commented Dec 28, 2020 •

edited

Loading