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: don't cork noop .end() by ronag · Pull Request #36633 · nodejs/node

Conversation

@ronag

@ronag ronag commented Dec 26, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kachkaev and bricss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added the http Issues and PRs related to the http subsystem. label Dec 26, 2020
@ronag
ronag force-pushed the fix-36620 branch from 45984ac to 4cc48de Compare December 26, 2020 10:13
@ronag
ronag requested review from mcollina and rickyes December 26, 2020 10:13
@ronag

ronag commented Dec 26, 2020

Copy link
Copy Markdown
Member Author

@nodejs/http
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag added v14.x and removed v14.x labels Dec 26, 2020
@ronag

ronag commented Dec 26, 2020

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina requested changes Dec 26, 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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/_http_outgoing.js Resolved

Uh oh!

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

kachkaev commented Dec 26, 2020

Copy link
Copy Markdown

@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 ??
All reactions

Sorry, something went wrong.

Uh oh!

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

ronag commented Dec 26, 2020

Copy link
Copy Markdown
Member Author

@kachkaev Sorry, I'm not familiar enough with that.
kachkaev reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

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

mcollina commented Dec 26, 2020

Copy link
Copy Markdown
Member

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.
kachkaev reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

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

ronag commented Dec 26, 2020

Copy link
Copy Markdown
Member Author

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.
kachkaev 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.
@mcollina

mcollina commented Dec 26, 2020

Copy link
Copy Markdown
Member

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 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.
benjamingr
benjamingr approved these changes Dec 26, 2020
View reviewed changes

@benjamingr benjamingr 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 with the test matteo asked for

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ronag ronag added v14.x labels Dec 27, 2020
@ronag

ronag commented Dec 27, 2020

Copy link
Copy Markdown
Member Author

Might need a separate backport for v14.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kachkaev kachkaev mentioned this pull request Dec 27, 2020
Calling .end on an already finished http response leaves the socket instance corked #36650
Closed
mitsos1os
mitsos1os reviewed Dec 27, 2020
View reviewed changes
Comment thread lib/_http_outgoing.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
mitsos1os
mitsos1os reviewed Dec 28, 2020
View reviewed changes
Comment thread lib/_http_outgoing.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@kachkaev 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 MylesBorins added the lts-watch-v14.x label Jan 5, 2021
@mitsos1os

mitsos1os commented Jan 8, 2021

Copy link
Copy Markdown
Contributor

Hi, is there any progress with PR?
kachkaev 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.
@ronag

ronag commented Jan 8, 2021

Copy link
Copy Markdown
Member Author

It's missing a test.
All reactions

Sorry, something went wrong.

Uh oh!

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

ronag commented Jan 9, 2021

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

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

mitsos1os commented Jan 9, 2021

Copy link
Copy Markdown
Contributor

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?
All reactions

Sorry, something went wrong.

Uh oh!

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

ronag commented Jan 9, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

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.
mitsos1os 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.
@mcollina

mcollina commented Jan 10, 2021

Copy link
Copy Markdown
Member

Conservatively putting it as semver-major.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Jan 10, 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
@ronag

ronag commented Jan 11, 2021

Copy link
Copy Markdown
Member Author

@mcollina i disagree with semver major. This is a fix for a rather serious bug. 🤔
kachkaev and mcollina reacted with thumbs up emoji
All reactions
  • 👍 2 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 11, 2021

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

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

kachkaev commented Jan 11, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

@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.
mitsos1os and pganapat reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 11, 2021
@mcollina

mcollina commented Jan 11, 2021

Copy link
Copy Markdown
Member

Agreed, thanks for context.
kachkaev 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.
danielleadams
danielleadams approved these changes Jan 12, 2021
View reviewed changes
@danielleadams

danielleadams commented Jan 12, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag and others added 2 commits January 12, 2021 10:58
@ronag
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
@mitsos1os @ronag
test: http complete response after socket double end
340a016
@ronag
ronag force-pushed the fix-36620 branch from 76677af to 340a016 Compare January 12, 2021 09:59
ronag added a commit that referenced this pull request Jan 12, 2021
@ronag
http: don't cork noop .end()
1b669b2 
Calling .end() a second time should be a noop and not
leave the socket corked.

Fixes: #36620

PR-URL: #36633
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
ronag pushed a commit that referenced this pull request Jan 12, 2021
@mitsos1os @ronag
test: http complete response after socket double end
ec794f9 
PR-URL: #36633
Fixes: #36620
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
@ronag

ronag commented Jan 12, 2021

Copy link
Copy Markdown
Member Author

Landed in 2af43f6...ec794f9
kachkaev and danielleadams reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag closed this Jan 12, 2021
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@ronag @danielleadams
http: don't cork noop .end()
a9a2dd3 
Calling .end() a second time should be a noop and not
leave the socket corked.

Fixes: #36620

PR-URL: #36633
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@mitsos1os @danielleadams
test: http complete response after socket double end
17a5233 
PR-URL: #36633
Fixes: #36620
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
v15.6.0 proposal #36889
Merged
mitsos1os added a commit to mitsos1os/node that referenced this pull request Jan 15, 2021
@mitsos1os
test: http complete response after socket double end
71aed6b 
PR-URL: nodejs#36633
Fixes: nodejs#36620
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
(cherry picked from commit ec794f9)
@mitsos1os mitsos1os mentioned this pull request Jan 15, 2021
[v14.x Backport] http: don't cork noop .end() #36940
Closed
@mitsos1os

mitsos1os commented Jan 15, 2021

Copy link
Copy Markdown
Contributor

I have also issued the backport PR for v14 branch here: #36940
kachkaev reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
BethGriggs pushed a commit that referenced this pull request Jan 26, 2021
@mitsos1os @BethGriggs
test: http complete response after socket double end
f002f23 
PR-URL: #36633
Backport-PR-URL: #36940
Fixes: #36620
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jan 26, 2021
@mitsos1os @BethGriggs
http: don't cork .end when not needed
5593187 
PR-URL: #36633
Backport-PR-URL: #36940
Fixes: #36620
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Jan 26, 2021
v14.15.5 proposal #37074
Merged
BethGriggs pushed a commit that referenced this pull request Jan 28, 2021
@mitsos1os @BethGriggs
test: http complete response after socket double end
c03cddb 
PR-URL: #36633
Backport-PR-URL: #36940
Fixes: #36620
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jan 28, 2021
@mitsos1os @BethGriggs
http: don't cork .end when not needed
25a3204 
PR-URL: #36633
Backport-PR-URL: #36940
Fixes: #36620
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Danielle Adams <[email protected]>
@targos targos added backported-to-v14.x and removed lts-watch-v14.x labels Apr 25, 2021
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
@benjamingr benjamingr benjamingr approved these changes
@rickyes rickyes Awaiting requested review from rickyes
+2 more reviewers
@mitsos1os mitsos1os mitsos1os left review comments
@danielleadams danielleadams danielleadams approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

http Issues and PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Calling res.end() twice stalls follow-up HTTP request (drain event is missing)

Uh oh!

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

9 participants

@ronag @kachkaev @mcollina @mitsos1os @nodejs-github-bot @danielleadams @benjamingr @MylesBorins @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.