quic: add proper error codes & messages for QUIC failures

[pimterry]

This wraps QUIC errors, providing useful error codes and messages for the defined error code cases from both OpenSSL and ngtcp2.

Before this, QUIC errors looked like:

// Application error with an explicit reason string:
Error [ERR_QUIC_APPLICATION_ERROR]: A QUIC application error occurred. 42n [client shutdown]
    at [kFinishClose] (node:internal/quic/quic:3505:22)
    at Session.onSessionClose (node:internal/quic/quic:679:31) {
  code: 'ERR_QUIC_APPLICATION_ERROR'
}

// All our internal errors:
Error [ERR_QUIC_TRANSPORT_ERROR]: A QUIC transport error occurred. 296n [undefined]
    at [kFinishClose] (node:internal/quic/quic:3502:22)
    at Session.onSessionClose (node:internal/quic/quic:679:31) {
  code: 'ERR_QUIC_TRANSPORT_ERROR'
}

Now they look like:

Error [ERR_QUIC_APPLICATION_ERROR]: QUIC application error 42: client shutdown
    at makeQuicError (node:internal/quic/quic:999:15)
    at [kFinishClose] (node:internal/quic/quic:3552:22)
    at Session.onSessionClose (node:internal/quic/quic:681:31) {
  code: 'ERR_QUIC_APPLICATION_ERROR',
  errorCode: 42n,
  type: 'application',
  reason: 'client shutdown'
}

Error [ERR_QUIC_TRANSPORT_ERROR]: QUIC transport error handshake failure (296)
    at makeQuicError (node:internal/quic/quic:988:15)
    at [kFinishClose] (node:internal/quic/quic:3534:22)
    at Session.onSessionClose (node:internal/quic/quic:681:31) {
  code: 'ERR_QUIC_TRANSPORT_ERROR',
  errorCode: 296n,
  type: 'transport',
  errorName: 'handshake failure'
}

[nodejs-github-bot]

Review requested:

• @nodejs/quic

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.29%. Comparing base (8257091) to head (c5bd305).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63198      +/-   ##
==========================================
- Coverage   90.32%   90.29%   -0.03%     
==========================================
  Files         730      730              
  Lines      234671   234825     +154     
  Branches    43946    43956      +10     
==========================================
+ Hits       211965   212043      +78     
- Misses      14423    14492      +69     
- Partials     8283     8290       +7     

Files with missing lines	Coverage Δ
lib/internal/errors.js	97.65% <ø> (-0.01%)	⬇️
lib/internal/quic/quic.js	100.00% <100.00%> (ø)

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Ethan-Arrowood]

this is a great change.

[Ethan-Arrowood]

this is a great change.

[pimterry]

Updated to use bindingdata, rebased and force pushed because we need to update the test from #63193 (just merged) as well since that asserted on the error message.

[metcoder95]

lgtm

[Qard]

Is there some reason you're undid the use of the existing core errors system? Shouldn't we be keeping that?

[Qard]

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.

[pimterry]

This was a suggestion here, changed here.

We still use standard error codes, but we use a single QuicError class instead of the standard construction of per-code error classes: https://github.com/nodejs/node/blob/main/doc/api/quic.md#class-quicerror. This error class is used by users as well to control QUIC error behaviour, and this approach gives us consistent errors for every kind of failure at the protocol level. I don't feel strongly about it myself, but it seemed like a reasonable suggestion and having a single QUIC error class is tidy.

What do we lose by not using the standard error mechanism vs this setup?

[Qard]

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.

[pimterry]

In this case, I think the previous code didn't do this either - our validators do hide internal stack traces, but AFAICT the normal error code definitions don't do this unless you use HideStackFramesError explicitly.

It is easy to do though, and I agree it'd be a nice improvement - I've just pushed a commit that adds ErrorCaptureStackTrace(err, makeQuicError) here, which will omit these internal bits from the stack in the same way. This mirrors the equivalent setup for custom errors elsewhere like

node/lib/internal/fs/utils.js

Lines 350 to 363 in 92f48f4

	function handleErrorFromBinding(ctx) {
	if (ctx.errno !== undefined) { // libuv error numbers
	const err = new UVException(ctx);
	ErrorCaptureStackTrace(err, handleErrorFromBinding);
	throw err;
	}
	if (ctx.error !== undefined) { // Errors created in C++ land.
	// TODO(joyeecheung): currently, ctx.error are encoding errors
	// usually caused by memory problems. We need to figure out proper error
	// code(s) for this.
	ErrorCaptureStackTrace(ctx.error, handleErrorFromBinding);
	throw ctx.error;
	}
	}

for FS bindings.

Is that what you were looking for?

[Qard]

Linter seems to be complaining, but otherwise LGTM.

[pimterry]

Linter seems to be complaining, but otherwise LGTM.

Yeah, linting passed in isolation but failed on the merge commit due to new errors added elsewhere.

I've now rebased, fixed the conflicting changes so everything'll hopefully pass, and force pushed. If you have a second to re-review @Qard I'd appreciate it! 🙏 Thanks

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/73815/

[nodejs-github-bot]

Landed in e1ae3a5