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

explicitly destroy sockets on clientError #3461

Merged
merged 1 commit into from May 30, 2017
Merged

explicitly destroy sockets on clientError #3461

merged 1 commit into from May 30, 2017

Conversation

@nlf
Copy link
Member

@nlf nlf commented Mar 21, 2017

node versions >= 6 no longer destroy sockets by default when a clientError listener is attached, we have to do so ourselves

node versions >= 6 no longer destroy sockets by default when a clientError listener is attached, we have to do so ourselves
@devinivy
Copy link
Member

@devinivy devinivy commented Mar 21, 2017

Good catch. Is that well-documented anywhere? I wonder if it will help fix server hangs, i.e. in #3290. Also– no problem with this code "re-destroying" the socket for node v4 and v5?

@nlf
Copy link
Member Author

@nlf nlf commented Mar 21, 2017

kind of? https://nodejs.org/api/http.html#http_event_clienterror click the little "history" arrow

@nlf
Copy link
Member Author

@nlf nlf commented Mar 21, 2017

and no, the destroy() method returns early if the socket is already destroyed

@hueniverse hueniverse self-assigned this May 30, 2017
@hueniverse hueniverse added this to the 16.3.0 milestone May 30, 2017
@hueniverse hueniverse merged commit 3d4f0fc into master May 30, 2017
3 checks passed
@hueniverse hueniverse deleted the socket-destroy branch May 30, 2017
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants