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.

Sign up for GitHub

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

Jump to bottom

tls: better error message for socket disconnect #18989

Closed
wants to merge 1 commit into from
Closed

tls: better error message for socket disconnect #18989

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

The error emitted when a connection is closed before the
TLS handshake completes seemed rather unspefic by just saying
socket hang up.

Use a more verbose message, that also indicates that this is
a purely client-side error, and remove a misleading comment.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

tls

@addaleax
tls: better error message for socket disconnect
f181674 
The error emitted when a connection is closed before the
TLS handshake completes seemed rather unspefic by just saying
`socket hang up`.

Use a more verbose message, that also indicates that this is
a purely client-side error, and remove a misleading comment.
@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Feb 25, 2018
@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/16490/

joyeecheung
joyeecheung approved these changes Feb 26, 2018
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

There are actually 4 errors with the same message (socket hang up) in lib/...would be even better if the error creation is put into a helper in lib/internal/errors.js and reuse it in other places.

@lpinca
Copy link
Member

lpinca commented Feb 26, 2018

Yes that error message is also used in the plain HTTP client if the socket if destroyed before response headers are read. The meaning is different from the one used here though.

jasnell
jasnell reviewed Feb 26, 2018
View reviewed changes
lib/_tls_wrap.js
@@ -1083,7 +1082,8 @@ function onConnectEnd() {
if (!this._hadError) {
const options = this[kConnectOptions];
this._hadError = true;
const error = new Error('socket hang up');
const error = new Error('Client network socket disconnected before ' +
'secure TLS connection was established');
Copy link
Member

Choose a reason for hiding this comment

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

I can't remember if we're keeping this intentionally with the old errors or if this needs to be converted to use internal/errors

Copy link
Member

Choose a reason for hiding this comment

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

I suggest to switch to the internal/errors and override the code for now. It is not a strong opinion though and I am fine with keeping it as it is for now as well.

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 going to leave this out of this PR.

I don’t think I’m a fan of changing the code property of exactly those errors where we always provided one, though.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 26, 2018
@jasnell
Copy link
Member

jasnell commented Feb 26, 2018

semver-major because of the error message change.

@addaleax
Copy link
Member Author

@jasnell @joyeecheung I’m not sure, do we have a story for these kinds of errors, that have a code property for parity with system errors? Is changing the error message for those always going to be considered semver-major?

@jasnell
Copy link
Member

jasnell commented Feb 26, 2018

I don't have a great answer for that either.

@joyeecheung
Copy link
Member

@addaleax I've done a few of those in c0762c2 . Although I've only moved the creation code into helpers inside internal/errors.js, the errors themselves are still the same as they were.

It does not have to be semver-major if the message is not changed, but that's what this PR is about, so I think it'll still be semver-major.

@joyeecheung
Copy link
Member

joyeecheung commented Feb 27, 2018 •
edited
Loading

Also, these errors all have err.code already (ECONNRESET) so we cannot easily convert them to ERR_* errors without some sorts of deprecation or ecosystem usage investigation anyway..

@BridgeAR
Copy link
Member

BridgeAR commented Mar 1, 2018

@joyeecheung I actually guess that quite some code in the wild relies on the old code property. So deprecating this is probably difficult. But on the other hand it will always conflict with our error codes...

BridgeAR
BridgeAR approved these changes Mar 1, 2018
View reviewed changes
lib/_tls_wrap.js
@@ -1083,7 +1082,8 @@ function onConnectEnd() {
if (!this._hadError) {
const options = this[kConnectOptions];
this._hadError = true;
const error = new Error('socket hang up');
const error = new Error('Client network socket disconnected before ' +
'secure TLS connection was established');
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to switch to the internal/errors and override the code for now. It is not a strong opinion though and I am fine with keeping it as it is for now as well.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2018
@addaleax
Copy link
Member Author

addaleax commented Mar 2, 2018

CI: https://ci.nodejs.org/job/node-test-commit/16598/

@apapirovski
Copy link
Member

Landed in eda7021

@apapirovski apapirovski closed this Mar 4, 2018
apapirovski pushed a commit that referenced this pull request Mar 4, 2018
@addaleax @apapirovski
tls: better error message for socket disconnect
eda7021 
The error emitted when a connection is closed before the
TLS handshake completes seemed rather unspefic by just saying
`socket hang up`.

Use a more verbose message, that also indicates that this is
a purely client-side error, and remove a misleading comment.

PR-URL: #18989
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@addaleax @MayaLekova
tls: better error message for socket disconnect
a4b7141 
The error emitted when a connection is closed before the
TLS handshake completes seemed rather unspefic by just saying
`socket hang up`.

Use a more verbose message, that also indicates that this is
a purely client-side error, and remove a misleading comment.

PR-URL: nodejs#18989
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@addaleax @lpinca @jasnell @joyeecheung @BridgeAR @apapirovski @cjihrig @nodejs-github-bot