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

tls: use .destroy(err) instead of destroy+emit #1711

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented May 15, 2015

Emit errors using .destroy(err) instead of .destroy() and
.emit('error', err). Otherwise close event is emitted with the
error argument set to false, even if the connection was torn down
because of the error.

See: #1119

Emit errors using `.destroy(err)` instead of `.destroy()` and
`.emit('error', err)`. Otherwise `close` event is emitted with the
`error` argument set to `false`, even if the connection was torn down
because of the error.

See: nodejs#1119
@Fishrock123 Fishrock123 added the tls Issues and PRs related to the tls subsystem. label May 15, 2015
@chrisdickinson
Copy link
Contributor

LGTM.

indutny added a commit that referenced this pull request May 22, 2015
Emit errors using `.destroy(err)` instead of `.destroy()` and
`.emit('error', err)`. Otherwise `close` event is emitted with the
`error` argument set to `false`, even if the connection was torn down
because of the error.

See: #1119
PR-URL: #1711
Reviewed-By: Chris Dickinson <[email protected]>
@indutny
Copy link
Member Author

indutny commented May 22, 2015

Landed in 80342f6, thank you!

@indutny indutny closed this May 22, 2015
@indutny indutny deleted the fix/tls-close-error-event branch May 22, 2015 11:28
indutny pushed a commit that referenced this pull request May 22, 2015
This fixes a race condition introduced in 80342f6.
`socket.destroy(err)` only emits the passed error when
`socket._writableState.errorEmitted === false`, `ssl.onerror`
sets `errorEmitted = true` just before calling
`socket.destroy()`.

See: #1119
See: #1711
PR-URL: #1769
Reviewed-By: Fedor Indutny <[email protected]>
@rvagg rvagg mentioned this pull request May 23, 2015
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
Emit errors using `.destroy(err)` instead of `.destroy()` and
`.emit('error', err)`. Otherwise `close` event is emitted with the
`error` argument set to `false`, even if the connection was torn down
because of the error.

See: nodejs/node#1119
PR-URL: nodejs/node#1711
Reviewed-By: Chris Dickinson <[email protected]>
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
This fixes a race condition introduced in 80342f6.
`socket.destroy(err)` only emits the passed error when
`socket._writableState.errorEmitted === false`, `ssl.onerror`
sets `errorEmitted = true` just before calling
`socket.destroy()`.

See: nodejs/node#1119
See: nodejs/node#1711
PR-URL: nodejs/node#1769
Reviewed-By: Fedor Indutny <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants