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
base: master
from

Conversation

Projects
None yet
9 participants
@indutny
Member

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

@mcollina

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

indutny commented Oct 12, 2016

Added test.

@Trott

This comment has been minimized.

Member

Trott commented Oct 12, 2016

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

net: fix ambiguity in EOF handling
`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 indutny:fix/ambiguity-in-net branch to 6e38e76 Oct 12, 2016

@indutny

This comment has been minimized.

Member

indutny commented Oct 12, 2016

@Trott sorry, just pushed it!

@indutny

This comment has been minimized.

@Trott

This comment has been minimized.

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

This comment has been minimized.

Member

Trott commented Oct 12, 2016

LGTM if no CI oddities arise.

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

This comment has been minimized.

@santigimeno

santigimeno Oct 12, 2016

Member

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.

This comment has been minimized.

@indutny

indutny Oct 12, 2016

Member

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

@mcollina

This comment has been minimized.

Member

mcollina commented Oct 13, 2016

LGTM

@jasnell

LGTM with green CI

@imyller

LGTM

@indutny

This comment has been minimized.

Member

indutny commented Oct 14, 2016

Landed in 31196ea, thank you!

@indutny indutny closed this Oct 14, 2016

@indutny indutny deleted the indutny:fix/ambiguity-in-net branch Oct 14, 2016

indutny added a commit that referenced this pull request Oct 14, 2016

net: fix ambiguity in EOF handling
`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 added a commit that referenced this pull request Oct 14, 2016

net: fix ambiguity in EOF handling
`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

This comment has been minimized.

Member

MylesBorins commented Nov 11, 2016

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

@indutny

This comment has been minimized.

Member

indutny commented Nov 11, 2016

MylesBorins added a commit that referenced this pull request Nov 11, 2016

net: fix ambiguity in EOF handling
`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 added a commit that referenced this pull request Nov 11, 2016

net: fix ambiguity in EOF handling
`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