lib: reuse invalid state errors on webstreams

[RafaelGSS]

We are tracking internally the webstreams performance and after some investigation (nodejs/undici#1203, nodejs/performance#9 (comment)), we found that one of the bottlenecks is the NodeError creation.

• I'm using undici.fetch as a real use case of web streams, the benchmark files are available at https://github.com/RafaelGSS/nodejs-webstreams-perf/blob/main/bench/fetch.js
• This patch improves the undici.fetch performance by approximately 23%.
  • Important: I'm considering only the benchmark above.
• I did all the tests on my developer machine and on a dedicated server, and the results seem consistent.

I'm not quite sure about the security impacts of this change, considering we're going to use the same object for all release calls.

More information on this performance work can be found at nodejs/performance#9.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[mcollina]

What's the stack trace of this error? Make sure it does should not include any user lines.

[RafaelGSS]

What's the stack trace of this error? Make sure it does should not include any user lines.

node:internal/errors:490
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_INVALID_STATE]: Invalid state: Reader released
    at new NodeError (node:internal/errors:399:5)
    at lazyReadableReleasedError (node:internal/webstreams/readablestream:147:21)
    at readableStreamReaderGenericRelease (node:internal/webstreams/readablestream:2078:30)
    at readableStreamDefaultReaderRelease (node:internal/webstreams/readablestream:2042:3)
    at ReadableStreamDefaultReader.releaseLock (node:internal/webstreams/readablestream:832:5)
    at main (/home/rafaelgss/repos/os/node/t.js:14:10) {
  code: 'ERR_INVALID_STATE'
}

I can remove the lazyReadableReleasedError from the stack trace using the hideStackFrames, but considering this won't show user lines, I think we are good.

UPDATE: I've updated to use hideStackFrames anyway.

[mcollina]

what's the final stack trace for these then? What's t.js in your stacktrace above?

[RafaelGSS]

Stacktrace

rafaelgss@rafaelgss-desktop:~/repos/os/node$ ./node t.js
node:internal/errors:490
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_INVALID_STATE]: Invalid state: Reader released
    at readableStreamReaderGenericRelease (node:internal/webstreams/readablestream:2079:30)
    at readableStreamDefaultReaderRelease (node:internal/webstreams/readablestream:2043:3)
    at ReadableStreamDefaultReader.releaseLock (node:internal/webstreams/readablestream:833:5)
    at main (/home/rafaelgss/repos/os/node/t.js:13:10) {
  code: 'ERR_INVALID_STATE'
}

Node.js v20.0.0-pre

t.js

const { ReadableStream } = require('node:stream/web')

async function main() {
  const stream = new ReadableStream({
    start(controller) {
      controller.close();
    },
  });

  const reader = stream.getReader();

  await reader.closed;
  reader.releaseLock(); // the error is created here
  await reader.closed; // this should throw the invalid state error
}

main()

Reference: https://github.com/nodejs/node/blob/main/test/parallel/test-whatwg-readablestream.js#L479

[mcollina]

Unfortunately, you'd need to remove everything that's not internal, otherwise users could be utterly confused.

[RafaelGSS]

@mcollina What about it now?

rafaelgss@rafaelgss-desktop:~/repos/os/node$ ./node t.js
TypeError [ERR_INVALID_STATE]: Invalid state: Reader released
    at new NodeError (node:internal/errors:399:5)
    at lazyReadableReleasedError (node:internal/webstreams/readablestream:152:19)
    at readableStreamReaderGenericRelease (node:internal/webstreams/readablestream:2094:30)
    at readableStreamDefaultReaderRelease (node:internal/webstreams/readablestream:2058:3)
    at ReadableStreamDefaultReader.releaseLock (node:internal/webstreams/readablestream:848:5)
 {
  code: 'ERR_INVALID_STATE'
}

[BridgeAR]

This is a good start but it won't match all internal frames. There are e.g., not always brackets involved.
The best one to match I could come up with was: const coreModuleRegExp = /^ {4}at (?:[^/\\(]+ \(|)node:(.+):\d+:\d+\)?$/;

[RafaelGSS]

Thanks! I've changed it a bit to perform the inverse operation (userland modules). Can you double-check?

[mcollina]

lgtm

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 5d50b84