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: emit close on unconnected socket #29803

Closed
wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Oct 1, 2019

Socket should always emit 'close'. Regardless whether it has been connected or not.

Probably a minor fix for an edge case.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Oct 1, 2019
Socket should always emit 'close'. Regardless
whether it has been connected or not.
@ronag ronag force-pushed the fix-net-destroy-close branch from 39e179b to 2b81112 Compare October 1, 2019 18:22
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Probably a minor fix for an edge case.

I agree.

}

cb(exception);
Copy link
Member

Choose a reason for hiding this comment

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

Does this moving this into the individual branches of the if matter, if emitCloseNT is delayed by a tick anyway?

Copy link
Member Author

@ronag ronag Oct 1, 2019

Choose a reason for hiding this comment

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

@addaleax: yes, if the user inside the callback wants to schedule something before 'close' using a nextTick.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'm very close the edge on the edge case on this one :)

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 3, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 3, 2019
@nodejs-github-bot
Copy link
Collaborator

Trott pushed a commit that referenced this pull request Oct 3, 2019
Socket should always emit 'close'. Regardless
whether it has been connected or not.

PR-URL: #29803
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@Trott
Copy link
Member

Trott commented Oct 3, 2019

Landed in a57da27

@Trott Trott closed this Oct 3, 2019
BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
Socket should always emit 'close'. Regardless
whether it has been connected or not.

PR-URL: #29803
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@BridgeAR BridgeAR mentioned this pull request Oct 10, 2019
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.

7 participants