Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

mitsos1os
Copy link
Contributor

@mitsos1os mitsos1os commented Jan 15, 2021 •
edited
Loading

Backport to v14 of #36633
It has been merged to latest v15 but for the time being not in the v14.x branch.

Some changes needed because code in _http_outgoing.js and tests is different between versions v14 and v15

@mitsos1os
test: http complete response after socket double end
71aed6b 
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
test: assert http writable corked state after end
5f944b9
@mitsos1os
http: don't cork .end when not needed
dccef0f
@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. v14.x labels Jan 15, 2021
@mitsos1os mitsos1os mentioned this pull request Jan 15, 2021
Closed
4 tasks
@mitsos1os
Copy link
Contributor Author

I can't directly request a review on the PR, so according to the original PR reviewers, @mcollina @benjamingr @danielleadams @rickyes could you please take a look?

Thanks!

@mcollina
Copy link
Member

This needs a CITGM run before landing.

@mitsos1os
Copy link
Contributor Author

This needs a CITGM run before landing.

Ok thanks! Do I need to do anything else, or will the required labels be automatically added for CI testing?

@richardlau richardlau added the needs-citgm PRs that need a CITGM CI run. label Jan 16, 2021
@mcollina
Copy link
Member

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2570/ might do the trick

mcollina
mcollina previously approved these changes Jan 18, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm.

@mcollina
Copy link
Member

v14.x-staging CITGM because there are a few suspects: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2572/

@mcollina mcollina dismissed their stale review January 18, 2021 18:07

too quick

@mitsos1os
Copy link
Contributor Author

mitsos1os commented Jan 19, 2021 •
edited
Loading

@mcollina sorry but didn't get what "too quick" was referring to... 😄
Is CITGM OK or are there any problems?

mcollina
mcollina approved these changes Jan 19, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

I needed to check the result of CITGM on v14.x-staging

@mitsos1os
Copy link
Contributor Author

Do I need anything else to prep the PR or we are good to go now?

@mcollina
Copy link
Member

cc @BethGriggs @richardlau

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 26, 2021 •
edited by BethGriggs
Loading

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

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 <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
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 <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
@BethGriggs
Copy link
Member

Landed in 15a16cd...5593187 (v14.x-staging)

@BethGriggs BethGriggs closed this Jan 26, 2021
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 <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
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 <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
@snyk-bot snyk-bot mentioned this pull request Nov 8, 2022
Open
@137717unity 137717unity mentioned this pull request Nov 9, 2022
Open
@baby636 baby636 mentioned this pull request Nov 9, 2022
Open
@ron-a20 ron-a20 mentioned this pull request Nov 9, 2022
Open
@kissofire kissofire mentioned this pull request Mar 18, 2023
Open
@molllyn1 molllyn1 mentioned this pull request Jun 8, 2023
Open
This was referenced Jun 8, 2023
Open
Open
@ron-a20 ron-a20 mentioned this pull request Jun 8, 2023
Open
@baby636 baby636 mentioned this pull request Jun 9, 2023
Open
@137717unity 137717unity mentioned this pull request Jun 9, 2023
Open
@wedataintelligence wedataintelligence mentioned this pull request Jun 9, 2023
Open
@ron-a20 ron-a20 mentioned this pull request Jun 21, 2023
Open
@137717unity 137717unity mentioned this pull request Jun 21, 2023
Open
@wedataintelligence wedataintelligence mentioned this pull request Jun 21, 2023
Open
@baby636 baby636 mentioned this pull request Jun 23, 2023
Open
@kissofire kissofire mentioned this pull request Jun 25, 2023
Open
@ron-a20 ron-a20 mentioned this pull request Oct 1, 2023
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Oct 1, 2023
Open
@kissofire kissofire mentioned this pull request Oct 1, 2023
Open
@wedataintelligence wedataintelligence mentioned this pull request Oct 2, 2023
Open
@molllyn1 molllyn1 mentioned this pull request Oct 2, 2023
Open
@137717unity 137717unity mentioned this pull request Oct 2, 2023
Open
@baby636 baby636 mentioned this pull request Oct 2, 2023
Open
@wedataintelligence wedataintelligence mentioned this pull request Feb 23, 2024
Open
@kissofire kissofire mentioned this pull request Feb 23, 2024
Open
@molllyn1 molllyn1 mentioned this pull request Feb 23, 2024
Open
@baby636 baby636 mentioned this pull request Feb 23, 2024
Open
@137717unity 137717unity mentioned this pull request Feb 23, 2024
Open
@ron-a20 ron-a20 mentioned this pull request Feb 24, 2024
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Feb 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BethGriggs BethGriggs BethGriggs approved these changes

@richardlau richardlau richardlau approved these changes

@benjamingr benjamingr benjamingr approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. needs-citgm PRs that need a CITGM CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mitsos1os @mcollina @nodejs-github-bot @BethGriggs @benjamingr @richardlau