Skip to content

explicitly destroy sockets on clientError#3461

Merged
hueniverse merged 1 commit intomasterfrom
socket-destroy
May 30, 2017
Merged

explicitly destroy sockets on clientError#3461
hueniverse merged 1 commit intomasterfrom
socket-destroy

Conversation

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

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

nlf commented Mar 21, 2017

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

@nlf
Copy link
Copy Markdown
Member Author

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 the feature New functionality or improvement label 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
@hueniverse hueniverse deleted the socket-destroy branch May 30, 2017 20:29
@lock
Copy link
Copy Markdown

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

feature New functionality or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants