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

lib: reuse invalid state errors on webstreams #46086

Merged

Conversation

RafaelGSS
Copy link
Member

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.

@RafaelGSS RafaelGSS added performance Issues and PRs related to the performance of Node.js. web streams labels Jan 4, 2023
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 4, 2023
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

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

@RafaelGSS
Copy link
Member Author

RafaelGSS commented Jan 6, 2023

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.

@RafaelGSS RafaelGSS force-pushed the perf/reuse-webstreams-error branch 2 times, most recently from 97dff7d to 4540f14 Compare January 6, 2023 19:31
@mcollina
Copy link
Member

mcollina commented Jan 7, 2023

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

@RafaelGSS
Copy link
Member Author

RafaelGSS commented Jan 7, 2023

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
Copy link
Member

mcollina commented Jan 7, 2023

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

@RafaelGSS
Copy link
Member Author

@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'
}

releasedError = new ERR_INVALID_STATE.TypeError('Reader released');
// Avoid V8 leak and remove userland stackstrace
releasedError.stack = SideEffectFreeRegExpPrototypeSymbolReplace(
/^.*\((?!node:|internal)[^()]*\).*$/gm,
Copy link
Member

Choose a reason for hiding this comment

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

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+\)?$/;

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 11, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 11, 2023
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 13, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 13, 2023
@nodejs-github-bot nodejs-github-bot merged commit 5d50b84 into nodejs:main Jan 13, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 5d50b84

theanarkh pushed a commit to theanarkh/node that referenced this pull request Jan 13, 2023
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: nodejs#46086
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
theanarkh pushed a commit to theanarkh/node that referenced this pull request Jan 13, 2023
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: nodejs#46086
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
theanarkh pushed a commit to theanarkh/node that referenced this pull request Jan 13, 2023
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: nodejs#46086
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
RafaelGSS added a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: nodejs#46086
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
RafaelGSS added a commit that referenced this pull request Jan 20, 2023
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: #46086
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: #46086
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: #46086
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
debadree25 added a commit to debadree25/node that referenced this pull request May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants