GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — net: fix ambiguity in EOF handling by indutny · Pull Request #9066 · nodejs/node

Conversation

@indutny

@indutny indutny commented Oct 12, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Oct 12, 2016
@mcollina

mcollina commented Oct 12, 2016

Copy link
Copy Markdown
Member

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

if not, LGTM.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Oct 12, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny indutny mentioned this pull request Oct 12, 2016
Fix memory leak caused by misfiring callback spdy-http2/node-spdy#287
Closed
@indutny

indutny commented Oct 12, 2016

Copy link
Copy Markdown
Member Author

Added test.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Oct 12, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

indutny commented Oct 12, 2016

Copy link
Copy Markdown
Member Author

@Trott sorry, just pushed it!
Trott reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Oct 12, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Oct 12, 2016

Copy link
Copy Markdown
Member

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.)
indutny reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Oct 12, 2016

Copy link
Copy Markdown
Member

LGTM if no CI oddities arise.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
santigimeno
santigimeno reviewed Oct 12, 2016
View reviewed changes
Comment thread test/parallel/test-net-end-close.js

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

@santigimeno santigimeno Oct 12, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@indutny indutny Oct 12, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
santigimeno and mcollina reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@mcollina

mcollina commented Oct 13, 2016

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Oct 13, 2016
View reviewed changes
jasnell
jasnell approved these changes Oct 13, 2016
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
cjihrig
cjihrig approved these changes Oct 13, 2016
View reviewed changes
imyller
imyller approved these changes Oct 13, 2016
View reviewed changes

@imyller imyller left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@indutny

indutny commented Oct 14, 2016

Copy link
Copy Markdown
Member Author

Landed in 31196ea, thank you!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
@MylesBorins MylesBorins added lts-watch-v4.x labels Nov 11, 2016
@MylesBorins

MylesBorins commented Nov 11, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny

indutny commented Nov 11, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd yep
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
@indutny Myles Borins
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
@MylesBorins MylesBorins added land-on-v4.x labels Nov 11, 2016
@MylesBorins MylesBorins removed lts-watch-v4.x labels Nov 11, 2016
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
@indutny Myles Borins
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
This was referenced Nov 22, 2016
v6.9.2 proposal #9735
Merged
v4.7.0 proposal #9736
Merged
@aliscco aliscco mentioned this pull request Nov 29, 2023
[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#705
Open
@leonardoadame leonardoadame mentioned this pull request Nov 29, 2023
[Snyk] Fix for 4 vulnerabilities leonardoadame/node#246
Open
This was referenced Nov 30, 2023
[Snyk] Security upgrade eslint from 2.13.1 to 4.5.0 aliscco/alisco-node#826
Open
[Snyk] Fix for 3 vulnerabilities aliscco/alisco-node#846
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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@indutny @mcollina @Trott @MylesBorins @jasnell @santigimeno @imyller @cjihrig @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.