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

http - emit close last #15588

Closed
wants to merge 2 commits into from
Closed

http - emit close last #15588

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 24, 2017 •
edited by lpinca
Loading

Emit close event after all other events.

Refs: #15270

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Sep 24, 2017
@ronag ronag changed the title fix: emit close last http - emit close last Sep 24, 2017
lpinca
lpinca approved these changes Sep 24, 2017
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM but it needs a test.

@addaleax
Copy link
Member

@mcollina I vaguely remember you having a similar PR a while back? Can you take a look?

@jasnell jasnell requested a review from mcollina September 25, 2017 16:07
@mcollina
Copy link
Member

I think this is ok, but it needs a test and a CITGM run.

We should probably also verify manually if pump still works.

mcollina
mcollina requested changes Sep 25, 2017
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.

needs a test

@BridgeAR
Copy link
Member

Ping @ronag

@lpinca
Copy link
Member

lpinca commented Sep 28, 2017

@ronag I pushed a commit with a test to your branch so we can move this forward. Hope you don't mind.

@lpinca
Copy link
Member

lpinca commented Sep 28, 2017

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

jasnell
jasnell reviewed Sep 29, 2017
View reviewed changes
test/parallel/test-http-client-close-event.js Outdated
assert.strictEqual(err.constructor, Error);
assert.strictEqual(err.message, 'socket hang up');
assert.strictEqual(err.code, 'ECONNRESET');
req.on('close', common.mustCall(() => server.close()));
Copy link
Member

Choose a reason for hiding this comment

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

Moving the req.on('close' ... line out of the req.on('error' ... would make this slightly better. Then just add a boolean that is set in on('error') and checked in on('close')

Copy link
Member

Choose a reason for hiding this comment

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

Done.

jasnell
jasnell approved these changes Sep 29, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM. pinging @mcollina for another review.

@lpinca
Copy link
Member

lpinca commented Oct 16, 2017

Ping @mcollina

mcollina
mcollina approved these changes Oct 16, 2017
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 with CI and CITGM green:

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1011/

@mcollina
Copy link
Member

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

@jasnell
Copy link
Member

jasnell commented Oct 16, 2017

CI failures look unrelated.

@mcollina
Copy link
Member

@refack can you please check CITGM output?

@mcollina
Copy link
Member

CITGM master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1016/
CITGM pr 15588: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1018/

@mcollina
Copy link
Member

CITGM should be green (as far as I understand it). Let me land.

@mcollina mcollina added dont-land-on-v4.x baking-for-lts PRs that need to wait before landing in a LTS release. labels Oct 18, 2017
@mcollina
Copy link
Member

Out of pure prudence, I'm flagging this as dont-land-on-X and as "baking-for-lts". I would like to have this in 9 for a while before having it in 8 or lower.

@mcollina mcollina self-assigned this Oct 18, 2017
@mcollina
Copy link
Member

Landed as 5d99a9b

@mcollina mcollina closed this Oct 18, 2017
mcollina pushed a commit that referenced this pull request Oct 18, 2017
@ronag @mcollina
http: emit close as the last event in the client
5d99a9b 
Emit close event after all other events in the client, e.g.
error will be emitted before close.

PR-URL: #15588
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@apapirovski
Copy link
Member

@ronag Congrats on landing your first commit in Node.js core and becoming a Contributor!

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@ronag @addaleax
http: emit close as the last event in the client
d90b9fc 
Emit close event after all other events in the client, e.g.
error will be emitted before close.

PR-URL: nodejs/node#15588
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@ronag @addaleax
http: emit close as the last event in the client
60f10e8 
Emit close event after all other events in the client, e.g.
error will be emitted before close.

PR-URL: nodejs/node#15588
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@apapirovski apapirovski mentioned this pull request Apr 12, 2018
Closed
@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. and removed baking-for-lts PRs that need to wait before landing in a LTS release. dont-land-on-v8.x labels Aug 17, 2018
@MylesBorins
Copy link
Contributor

@mcollina should we consider this for 8.x?

@mcollina
Copy link
Member

no, this should not be backportes

@MylesBorins MylesBorins added dont-land-on-v8.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees

@mcollina mcollina

Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ronag @addaleax @mcollina @BridgeAR @lpinca @jasnell @apapirovski @MylesBorins @nodejs-github-bot