http: don't cork noop .end() #36633

ronag · 2020-12-26T10:12:44Z

Calling .end() a second time should be a noop and not
leave the socket corked.

Fixes: #36620

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

ronag · 2020-12-26T10:20:40Z

@nodejs/http

ronag · 2020-12-26T11:05:29Z

@kachkaev would you mind confirming whether this resolves your issue?

mcollina

Could you add a test for the 'drain' event as mentioned in the issue?

lib/_http_outgoing.js

kachkaev · 2020-12-26T14:46:38Z

@ronag many thanks for the PR! I’m happy to try it out, I just don’t have any experience using custom Node builds. Do you know if it’s possible to do something like this in GitHub workflows? That would be the easiest and show the results transparently to everyone.

      - name: Use Node.js (custom build)
        uses: actions/setup-node@v2
        with:
          node-version: ?? from this PR ??

ronag · 2020-12-26T14:48:36Z

@kachkaev Sorry, I'm not familiar enough with that.

mcollina · 2020-12-26T14:54:40Z

Do you know if it’s possible to do something like this in GitHub workflows?

Unfortunately no, we don't publish the builds from individual PRs.

ronag · 2020-12-26T19:27:33Z

Could you add a test for the 'drain' event as mentioned in the issue?

The bug is not that the drain event is missing. It's that the socket stays corked and doesn't write.

mcollina · 2020-12-26T19:39:53Z

The bug is not that the drain event is missing. It's that the socket stays corked and doesn't write.

Then add a test for the write :). I would like to have a test that checks the end-user behavior.

benjamingr

LGTM with the test matteo asked for

ronag · 2020-12-27T21:56:48Z

Might need a separate backport for v14.

lib/_http_outgoing.js

mitsos1os · 2021-01-08T11:53:23Z

Hi, is there any progress with PR?

ronag · 2021-01-08T14:33:07Z

It's missing a test.

ronag · 2021-01-09T14:54:05Z

The bug is not that the drain event is missing. It's that the socket stays corked and doesn't write.

Then add a test for the write :). I would like to have a test that checks the end-user behavior.

If someone wants to help with writing this test I think we could merge this sooner rather than later.

mitsos1os · 2021-01-09T15:29:52Z

The bug is not that the drain event is missing. It's that the socket stays corked and doesn't write.

Then add a test for the write :). I would like to have a test that checks the end-user behavior.

If someone wants to help with writing this test I think we could merge this sooner rather than later.

How would you like to help with the test? Could I submit to your repo?

ronag · 2021-01-09T15:53:42Z

How would you like to help with the test? Could I submit to your repo?

Just do your own fork, checkout a copy of this branch, and I can pull in your commit.

mcollina · 2021-01-10T22:14:46Z

Conservatively putting it as semver-major.

mcollina

lgtm

ronag · 2021-01-11T06:33:17Z

@mcollina i disagree with semver major. This is a fix for a rather serious bug. 🤔

nodejs-github-bot · 2021-01-11T16:55:47Z

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

kachkaev · 2021-01-11T18:44:04Z

@mcollina I agree with @ronag on that this is a bug in v14 & v15. The behaviour unexpectedly changed between v13 and v14 according to my MWE: https://github.com/kachkaev/node-http-response-double-end-call-breaking-drain-event/runs/1606418629

It took me more than two working days in investigate the problem and create #36633 (it involved quite a few layers of abstraction to shave off). I wish others no longer fall into the same trap after upgrading to Node 14 from v12. Calling res.end() twice in Node < v14.0.0 was not punished or warned about.

mcollina · 2021-01-11T22:24:41Z

Agreed, thanks for context.

danielleadams · 2021-01-12T03:26:24Z

@ronag do you mind consolidating bb17da1 and 085bb3a - I'm getting an issue landing the second commit since it is merged into this PR without a commit message.

Calling .end() a second time should be a noop and not leave the socket corked. Fixes: nodejs#36620

Calling .end() a second time should be a noop and not leave the socket corked. Fixes: #36620 PR-URL: #36633 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>

PR-URL: #36633 Fixes: #36620 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>

ronag · 2021-01-12T10:01:14Z

Landed in 2af43f6...ec794f9

Calling .end() a second time should be a noop and not leave the socket corked. Fixes: #36620 PR-URL: #36633 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>

PR-URL: #36633 Fixes: #36620 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>

PR-URL: nodejs#36633 Fixes: nodejs#36620 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Danielle Adams <adamzdanielle@gmail.com> (cherry picked from commit ec794f9)

mitsos1os · 2021-01-15T13:41:18Z

I have also issued the backport PR for v14 branch here: #36940

PR-URL: #36633 Backport-PR-URL: #36940 Fixes: #36620 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Dec 26, 2020

ronag force-pushed the fix-36620 branch from 45984ac to 4cc48de Compare December 26, 2020 10:13

ronag requested review from rickyes and mcollina December 26, 2020 10:13

ronag added v14.x and removed v14.x labels Dec 26, 2020

mcollina requested changes Dec 26, 2020

View reviewed changes

lib/_http_outgoing.js Show resolved Hide resolved

benjamingr approved these changes Dec 26, 2020

View reviewed changes

ronag added v14.x labels Dec 27, 2020

kachkaev mentioned this pull request Dec 27, 2020

Calling .end on an already finished http response leaves the socket instance corked #36650

Closed

mitsos1os reviewed Dec 27, 2020

View reviewed changes

lib/_http_outgoing.js Show resolved Hide resolved

mitsos1os reviewed Dec 28, 2020

View reviewed changes

lib/_http_outgoing.js Show resolved Hide resolved

kachkaev mentioned this pull request Jan 4, 2021

Call res.write('') inside apiRes.redirect() helper to prevent an edge case vercel/next.js#20461

Merged

MylesBorins added the lts-watch-v14.x label Jan 5, 2021

mcollina approved these changes Jan 10, 2021

View reviewed changes

mcollina removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 11, 2021

danielleadams approved these changes Jan 12, 2021

View reviewed changes

ronag and others added 2 commits January 12, 2021 10:58

http: don't cork noop .end()

174fa33

Calling .end() a second time should be a noop and not leave the socket corked. Fixes: nodejs#36620

test: http complete response after socket double end

340a016

ronag force-pushed the fix-36620 branch from 76677af to 340a016 Compare January 12, 2021 09:59

ronag closed this Jan 12, 2021

danielleadams mentioned this pull request Jan 12, 2021

v15.6.0 proposal #36889

Merged

mitsos1os mentioned this pull request Jan 15, 2021

[v14.x Backport] http: don't cork noop .end() #36940

Closed

BethGriggs mentioned this pull request Jan 26, 2021

v14.15.5 proposal #37074

Merged

targos added backported-to-v14.x and removed lts-watch-v14.x labels Apr 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: don't cork noop .end() #36633

http: don't cork noop .end() #36633

ronag commented Dec 26, 2020

ronag commented Dec 26, 2020

ronag commented Dec 26, 2020

mcollina left a comment

kachkaev commented Dec 26, 2020

ronag commented Dec 26, 2020

mcollina commented Dec 26, 2020

ronag commented Dec 26, 2020

mcollina commented Dec 26, 2020

benjamingr left a comment

ronag commented Dec 27, 2020

mitsos1os commented Jan 8, 2021

ronag commented Jan 8, 2021

ronag commented Jan 9, 2021

mitsos1os commented Jan 9, 2021

ronag commented Jan 9, 2021 •

edited

Loading

mcollina commented Jan 10, 2021

mcollina left a comment

ronag commented Jan 11, 2021

nodejs-github-bot commented Jan 11, 2021

kachkaev commented Jan 11, 2021 •

edited

Loading

mcollina commented Jan 11, 2021

danielleadams commented Jan 12, 2021 •

edited

Loading

ronag commented Jan 12, 2021

mitsos1os commented Jan 15, 2021

http: don't cork noop .end() #36633

http: don't cork noop .end() #36633

Conversation

ronag commented Dec 26, 2020

Checklist

ronag commented Dec 26, 2020

ronag commented Dec 26, 2020

mcollina left a comment

Choose a reason for hiding this comment

kachkaev commented Dec 26, 2020

ronag commented Dec 26, 2020

mcollina commented Dec 26, 2020

ronag commented Dec 26, 2020

mcollina commented Dec 26, 2020

benjamingr left a comment

Choose a reason for hiding this comment

ronag commented Dec 27, 2020

mitsos1os commented Jan 8, 2021

ronag commented Jan 8, 2021

ronag commented Jan 9, 2021

mitsos1os commented Jan 9, 2021

ronag commented Jan 9, 2021 • edited Loading

mcollina commented Jan 10, 2021

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Jan 11, 2021

nodejs-github-bot commented Jan 11, 2021

kachkaev commented Jan 11, 2021 • edited Loading

mcollina commented Jan 11, 2021

danielleadams commented Jan 12, 2021 • edited Loading

ronag commented Jan 12, 2021

mitsos1os commented Jan 15, 2021

ronag commented Jan 9, 2021 •

edited

Loading

kachkaev commented Jan 11, 2021 •

edited

Loading

danielleadams commented Jan 12, 2021 •

edited

Loading