Skip to content

http: fix res emit close before user finish - #20941

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:fix-http-req-res-close-state
Closed

http: fix res emit close before user finish#20941
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:fix-http-req-res-close-state

Conversation

@ronag

@ronagronag commented May 24, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Fixes a regression caused by #20611 which breaks eos, pump and pumpify.

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

@nodejs-github-botnodejs-github-bot added the http Issues or PRs related to the http subsystem. label May 24, 2018
@ronag

ronag commented May 24, 2018

Copy link
Copy Markdown
MemberAuthor

@jasnell@trivikr This is potentially urgent for a 10.2.1?

@ronag

ronag commented May 24, 2018

Copy link
Copy Markdown
MemberAuthor

Another possible solution is to use prefinish instead of finish for resOnFinish but I'm unsure about the ramifications of that. This solution is safer.

@mafintosh

mafintosh commented May 24, 2018

Copy link
Copy Markdown
Member

@ronag Thanks for the quick fix :)

@mafintosh

mafintosh commented May 24, 2018

Copy link
Copy Markdown
Member

This is def urgent for a 10.2.1 - without this fix end-of-stream, pump and most userland stream stuff using those are broken when using http streams

mafintosh
mafintosh reviewed May 24, 2018
View reviewed changes
Comment threadlib/_http_server.js Outdated

@mafintoshmafintoshMay 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could also do process.nextTick(emitCloseNT, res) to avoid the bind and accept res as an arg in your emitCloseNT function

@mcollinamcollinaMay 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this will be better.

@jasnell

jasnell commented May 24, 2018

Copy link
Copy Markdown
Member

/cc @MylesBorins

@mafintosh

mafintosh commented May 24, 2018

Copy link
Copy Markdown
Member

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

@jasnelljasnell added the fast-track PRs that do not need to wait for 48 hours to land. label May 24, 2018
mcollina
mcollina approved these changes May 24, 2018
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@mcollina

mcollina commented May 24, 2018

Copy link
Copy Markdown
Member

+1 to fast tracking.

mafintosh
mafintosh reviewed May 24, 2018
View reviewed changes
Comment threadtest/parallel/test-http-req-res-close.js Outdated

@mafintoshmafintoshMay 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you have an unneeded paran around the arrow fn here which the linter complains about

@MylesBorinsMylesBorinsMay 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if everything else is green I'll go ahead and fix that while landing

@ronag
ronagforce-pushed the fix-http-req-res-close-state branch from 0119e3a to a07ea85CompareMay 24, 2018 15:50
@ronag

ronag commented May 24, 2018

Copy link
Copy Markdown
MemberAuthor

Fixed comments.

@jasnell

jasnell commented May 24, 2018

Copy link
Copy Markdown
Member

The other fix for 10.2.1 has been landed. As soon as this one lands we should be ready to go for 10.2.1

@MylesBorins

MylesBorins commented May 24, 2018

Copy link
Copy Markdown
Contributor

Kicked off another CI based on new changes

https://ci.nodejs.org/job/node-test-commit/18750/

BridgeAR
BridgeAR approved these changes May 24, 2018
View reviewed changes

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM but the comment should be addressed (it could also be removed while landing).

Comment threadtest/parallel/test-http-req-res-close.js
})));
res.on('close', common.mustCall(() => {
resClosed = true;
// assert.strictEqual(res._writableState.ended, true);

@BridgeARBridgeARMay 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please remove commented code.

@MylesBorins

MylesBorins commented May 24, 2018

Copy link
Copy Markdown
Contributor

The failures on arm will all infra related. I've fixed the linting error and removed comment when landing.

landed in 8ce20af

MylesBorins pushed a commit that referenced this pull request May 24, 2018
@ronag@MylesBorins
http: fix res emit close before user finish
8ce20af
PR-URL: #20941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 24, 2018
@ronag@MylesBorins
http: fix res emit close before user finish
2a9c833
PR-URL: #20941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request May 24, 2018
Merged
@mafintosh

mafintosh commented May 24, 2018

Copy link
Copy Markdown
Member

Thanks everyone

@benjamingrbenjamingr mentioned this pull request May 26, 2018
Closed
hekike pushed a commit to restify/node-restify that referenced this pull request Jun 6, 2018
Peter Marton
fix(restify): handle res close event in Node >= 10
929f9a1
Changes introduced in:
nodejs/node#20941nodejs/node#20611
@hekikehekike mentioned this pull request Jun 6, 2018
Merged
@brettkieferbrettkiefer mentioned this pull request Jun 8, 2018
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@jasnelljasnelljasnell approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+2 more reviewers
@mafintoshmafintoshmafintosh approved these changes
@MylesBorinsMylesBorinsMylesBorins left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fast-trackPRs that do not need to wait for 48 hours to land.httpIssues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ronag@mafintosh@jasnell@mcollina@MylesBorins@BridgeAR@targos@nodejs-github-bot