Skip to content

net: fix ambiguity in EOF handling#9066

Closed
indutny wants to merge 1 commit intonodejs:masterfrom
indutny:fix/ambiguity-in-net
Closed

net: fix ambiguity in EOF handling#9066
indutny wants to merge 1 commit intonodejs:masterfrom
indutny:fix/ambiguity-in-net

Conversation

@indutny
Copy link
Member

@indutny indutny commented Oct 12, 2016 •
edited
Loading

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

net

Description of change

end MUST always be emitted before close. However, if a handle
will invoke uv_close_cb immediately, or in the same JS tick - close
may be emitted first.

cc @nodejs/collaborators

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Oct 12, 2016
@mcollina
Copy link
Member

mcollina commented Oct 12, 2016

Nice one. I imagine there is very little chance to write a unit test for this, right?

if not, LGTM.

@indutny
Copy link
Member Author

indutny commented Oct 12, 2016

@mcollina I actually just got an idea on how to do this, but wanted to gather some preliminary feedback first.

@indutny indutny mentioned this pull request Oct 12, 2016
Closed
@indutny
Copy link
Member Author

indutny commented Oct 12, 2016

Added test.

@Trott
Copy link
Member

Trott commented Oct 12, 2016

@indutny Test not showing up here... (Forgot to push maybe? Or need to force-push maybe?)

@indutny
net: fix ambiguity in EOF handling
6e38e76 
`end` MUST always be emitted **before** `close`. However, if a handle
will invoke `uv_close_cb` immediately, or in the same JS tick - `close`
may be emitted first.
@indutny indutny force-pushed the fix/ambiguity-in-net branch from 9920f17 to 6e38e76 Compare October 12, 2016 22:36
@indutny
Copy link
Member Author

indutny commented Oct 12, 2016

@Trott sorry, just pushed it!

@indutny
Copy link
Member Author

indutny commented Oct 12, 2016

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

@Trott
Copy link
Member

Trott commented Oct 12, 2016

Stress test to make sure there's no unexpected flakiness hidden in the test somewhere:

https://ci.nodejs.org/job/node-stress-single-test/994/

(Failures on the two win2008* jobs can be ignored, I think.)

@Trott
Copy link
Member

Trott commented Oct 12, 2016

LGTM if no CI oddities arise.

santigimeno
santigimeno reviewed Oct 12, 2016
View reviewed changes
test/parallel/test-net-end-close.js

process.on('exit', () => {
assert.deepStrictEqual(events, [ 'end', 'close' ]);
});
Copy link
Member

@santigimeno santigimeno Oct 12, 2016

Choose a reason for hiding this comment

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

Would something like this work?

s.on('end', common.mustCall(() => {
  s.on('close', common.mustCall(() => {}));
}));

... so you can get rid of events and the exit listener.

Copy link
Member Author

@indutny indutny Oct 12, 2016

Choose a reason for hiding this comment

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

On one hand - yes, on the other - if this test will ever error, the error message will be more informative.

@mcollina
Copy link
Member

mcollina commented Oct 13, 2016

LGTM

jasnell
jasnell approved these changes Oct 13, 2016
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 with green CI

imyller
imyller approved these changes Oct 13, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@indutny
Copy link
Member Author

indutny commented Oct 14, 2016

Landed in 31196ea, thank you!

@indutny indutny closed this Oct 14, 2016
@indutny indutny deleted the fix/ambiguity-in-net branch October 14, 2016 13:40
indutny added a commit that referenced this pull request Oct 14, 2016
@indutny
net: fix ambiguity in EOF handling
31196ea 
`end` MUST always be emitted **before** `close`. However, if a handle
will invoke `uv_close_cb` immediately, or in the same JS tick - `close`
may be emitted first.

PR-URL: #9066
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
jasnell pushed a commit that referenced this pull request Oct 14, 2016
@indutny @jasnell
net: fix ambiguity in EOF handling
6f1cae7 
`end` MUST always be emitted **before** `close`. However, if a handle
will invoke `uv_close_cb` immediately, or in the same JS tick - `close`
may be emitted first.

PR-URL: #9066
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 11, 2016

@indutny this lands cleanly on v4 and v6. Is this something we should consider backporting?

@indutny
Copy link
Member Author

indutny commented Nov 11, 2016

@thealphanerd yep

MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
@indutny
net: fix ambiguity in EOF handling
9a192a9 
`end` MUST always be emitted **before** `close`. However, if a handle
will invoke `uv_close_cb` immediately, or in the same JS tick - `close`
may be emitted first.

PR-URL: #9066
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
@indutny
net: fix ambiguity in EOF handling
0f1a22d 
`end` MUST always be emitted **before** `close`. However, if a handle
will invoke `uv_close_cb` immediately, or in the same JS tick - `close`
may be emitted first.

PR-URL: #9066
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
This was referenced Nov 22, 2016
Merged
Merged
@aliscco aliscco mentioned this pull request Nov 29, 2023
Open
@leonardoadame leonardoadame mentioned this pull request Nov 29, 2023
Open
This was referenced Nov 30, 2023
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@santigimeno santigimeno santigimeno left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@imyller imyller imyller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

net Issues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@indutny @mcollina @Trott @MylesBorins @jasnell @santigimeno @imyller @cjihrig @nodejs-github-bot