-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
quic: add proper error codes & messages for QUIC failures #63198
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
base: main
Are you sure you want to change the base?
Changes from all commits
298d7d1
73bc7ad
89f0295
aea500a
60b6e3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,13 +104,11 @@ const { | |
| ERR_INVALID_THIS, | ||
| ERR_MISSING_ARGS, | ||
| ERR_OUT_OF_RANGE, | ||
| ERR_QUIC_APPLICATION_ERROR, | ||
| ERR_QUIC_CONNECTION_FAILED, | ||
| ERR_QUIC_ENDPOINT_CLOSED, | ||
| ERR_QUIC_OPEN_STREAM_FAILED, | ||
| ERR_QUIC_STREAM_ABORTED, | ||
| ERR_QUIC_STREAM_RESET, | ||
| ERR_QUIC_TRANSPORT_ERROR, | ||
| ERR_QUIC_VERSION_NEGOTIATION_ERROR, | ||
| }, | ||
| } = require('internal/errors'); | ||
|
|
@@ -673,10 +671,12 @@ setCallbacks({ | |
| * @param {number} errorType | ||
| * @param {number} code | ||
| * @param {string} [reason] | ||
| * @param {string} [errorName] Decoded TLS alert name when `code` is a | ||
| * CRYPTO_ERROR; otherwise undefined. | ||
| */ | ||
| onSessionClose(errorType, code, reason) { | ||
| debug('session close callback', errorType, code, reason); | ||
| this[kOwner][kFinishClose](errorType, code, reason); | ||
| onSessionClose(errorType, code, reason, errorName) { | ||
| debug('session close callback', errorType, code, reason, errorName); | ||
| this[kOwner][kFinishClose](errorType, code, reason, errorName); | ||
| }, | ||
|
|
||
| /** | ||
|
|
@@ -968,21 +968,49 @@ class QuicError extends Error { | |
| } | ||
| } | ||
|
|
||
| // Converts a raw QuicError array [type, code, reason] from C++ into a | ||
| // proper Node.js Error object. | ||
| // Build the human-readable message for an ERR_QUIC_TRANSPORT_ERROR or | ||
| // ERR_QUIC_APPLICATION_ERROR. `errorName` is the symbolic name for | ||
| // the wire code when known: either the OpenSSL-decoded TLS alert | ||
| // (CRYPTO_ERROR; 0x100..0x1ff) or one of the named transport codes | ||
| // from RFC 9000 (e.g. PROTOCOL_VIOLATION). Otherwise undefined. | ||
| // `reason` is the peer-supplied UTF-8 reason string from the | ||
| // CONNECTION_CLOSE / RESET_STREAM frame, often empty. | ||
| function quicErrorMessage(prefix, errorCode, reason, errorName) { | ||
| let msg = `${prefix} `; | ||
| msg += errorName ? `${errorName} (${errorCode})` : `${errorCode}`; | ||
| if (reason) msg += `: ${reason}`; | ||
| return msg; | ||
| } | ||
|
|
||
| function makeQuicError(code, prefix, type, errorCode, reason, errorName) { | ||
| const err = new QuicError( | ||
| quicErrorMessage(prefix, errorCode, reason, errorName), | ||
| { errorCode, code, type }); | ||
| if (reason) err.reason = reason; | ||
| if (errorName) err.errorName = errorName; | ||
| return err; | ||
| } | ||
|
Comment on lines
+985
to
+992
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not keep the same error construction mechanism as the others? We lose a bunch of things by not using that. Not sure we should be taking these out of the global errors set.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a suggestion here, changed here. We still use standard error codes, but we use a single What do we lose by not using the standard error mechanism vs this setup?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We lose the hideStackFrames logic, which means we'll see a bunch more internals things in stack traces here which we might not want to surface, like every validator gains a stack line without this. I'm not against this change, but just want to be sure that special-casing this is acceptable. |
||
|
|
||
| function convertQuicError(error) { | ||
| const type = error[0]; | ||
| const code = error[1]; | ||
| const reason = error[2]; | ||
| const errorName = error[3]; | ||
| switch (type) { | ||
| case 'transport': | ||
| return new ERR_QUIC_TRANSPORT_ERROR(code, reason); | ||
| return makeQuicError('ERR_QUIC_TRANSPORT_ERROR', | ||
| 'QUIC transport error', | ||
| 'transport', code, reason, errorName); | ||
| case 'application': | ||
| return new ERR_QUIC_APPLICATION_ERROR(code, reason); | ||
| return makeQuicError('ERR_QUIC_APPLICATION_ERROR', | ||
| 'QUIC application error', | ||
| 'application', code, reason, errorName); | ||
| case 'version_negotiation': | ||
| return new ERR_QUIC_VERSION_NEGOTIATION_ERROR(); | ||
| default: | ||
| return new ERR_QUIC_TRANSPORT_ERROR(code, reason); | ||
| return makeQuicError('ERR_QUIC_TRANSPORT_ERROR', | ||
| 'QUIC transport error', | ||
| 'transport', code, reason, errorName); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -3464,7 +3492,7 @@ class QuicSession { | |
| * @param {number} code | ||
| * @param {string} [reason] | ||
| */ | ||
| [kFinishClose](errorType, code, reason) { | ||
| [kFinishClose](errorType, code, reason, errorName) { | ||
| // If code is zero, then we closed without an error. Yay! We can destroy | ||
| // safely without specifying an error. | ||
| if (code === 0n) { | ||
|
|
@@ -3473,7 +3501,8 @@ class QuicSession { | |
| return; | ||
| } | ||
|
|
||
| debug('finishing closing the session with an error', errorType, code, reason); | ||
| debug('finishing closing the session with an error', | ||
| errorType, code, reason, errorName); | ||
|
|
||
| // If the local side initiated this close with an error code (via | ||
| // close({ code })), this is an intentional shutdown; not an error. | ||
|
|
@@ -3500,10 +3529,14 @@ class QuicSession { | |
| // session would leak with `closed` hanging forever. | ||
| switch (errorType) { | ||
| case 0: /* Transport Error */ | ||
| this.destroy(new ERR_QUIC_TRANSPORT_ERROR(code, reason)); | ||
| this.destroy(makeQuicError('ERR_QUIC_TRANSPORT_ERROR', | ||
| 'QUIC transport error', | ||
| 'transport', code, reason, errorName)); | ||
| break; | ||
| case 1: /* Application Error */ | ||
| this.destroy(new ERR_QUIC_APPLICATION_ERROR(code, reason)); | ||
| this.destroy(makeQuicError('ERR_QUIC_APPLICATION_ERROR', | ||
| 'QUIC application error', | ||
| 'application', code, reason, errorName)); | ||
| break; | ||
| case 2: /* Version Negotiation Error */ | ||
| this.destroy(new ERR_QUIC_VERSION_NEGOTIATION_ERROR()); | ||
|
|
@@ -3512,7 +3545,9 @@ class QuicSession { | |
| this.destroy(); | ||
| break; | ||
| default: | ||
| this.destroy(new ERR_QUIC_TRANSPORT_ERROR(code, reason)); | ||
| this.destroy(makeQuicError('ERR_QUIC_TRANSPORT_ERROR', | ||
| 'QUIC transport error', | ||
| 'transport', code, reason, errorName)); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.