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.

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

net: fix ambiguity in EOF handling #9066

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Oct 12, 2016

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

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
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?)

`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
Copy link
Member Author

indutny commented Oct 12, 2016

@Trott sorry, just pushed it!

@indutny
Copy link
Member Author

indutny commented Oct 12, 2016

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


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

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

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

LGTM

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

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
`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
`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

@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
`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
`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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants