Skip to content

Comments

fix(vitest): simplify hoist transform check regex to avoid expensive regex match#4974

Merged
sheremet-va merged 1 commit intovitest-dev:mainfrom
hi-ogawa:fix-regexpHoistable-dos
Jan 16, 2024
Merged

fix(vitest): simplify hoist transform check regex to avoid expensive regex match#4974
sheremet-va merged 1 commit intovitest-dev:mainfrom
hi-ogawa:fix-regexpHoistable-dos

Conversation

@hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Jan 16, 2024

Description

Since the regex change introduced in the above PR, certain source code with a huge inline source map leads to freeze main vitest process for expensive regexpHoistable.test(code) quite a long time. (See comment in #4855 (comment))

I'm not really familiar with estimating regex perf, but the arbitrary whitespace prefix [ \t]* seems to be bad and it's probably redundant since \b already asserts word boundary.

To reproduce this, build vitest itself with pnpm dev (which adds inline sourcemap), then run:

pnpm -C test/browser test-fixtures run

(... it gets stuck for a while since expensive regex test takes over main thread. waiting for a while to see...)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Errors ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

Vitest caught 1 unhandled error during the test run.
This might cause false positive tests. Resolve unhandled errors to make sure your tests are not affected.

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Error: [birpc] timeout on calling "getFiles"
 ❯ ../../../../../../../../../__vitest__/assets/index-0EYobN63.js:5:60901

This error originated in "basic.test.ts" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

It's also odd that Vitest's own file such as packages/vitest/dist/vendor/vi.smj1Ggd4.js goes through these transform, but I suppose that's a different issue with externalization during local dev.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

@netlify
Copy link

netlify bot commented Jan 16, 2024

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit 4863efc
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/65a63ebdd9138f0008c69710

@hi-ogawa hi-ogawa force-pushed the fix-regexpHoistable-dos branch from 60f7073 to 4863efc Compare January 16, 2024 08:30
@hi-ogawa hi-ogawa marked this pull request as ready for review January 16, 2024 08:45
@sheremet-va
Copy link
Member

It's also odd that Vitest's own file such as packages/vitest/dist/vendor/vi.smj1Ggd4.js goes through these transform, but I suppose that's a different issue with externalization during local dev.

All imported files go through a transformation when running in the browser. I am not sure what is odd here. Vitest is not bundled by the optimizer on purpose.

@hi-ogawa
Copy link
Contributor Author

All imported files go through a transformation when running in the browser. I am not sure what is odd here. Vitest is not bundled by the optimizer on purpose.

Yeah, you're right. I thought seeing many files vitest/dist/vendor/... are odd, but now I get that those files are genuinely imported from vitest/utils, browser, runners etc.... I'm still digesting how browser mode works. Thanks for clarifying it!


I forgot to mention but regexpHoistable.test(code) is still false for all internal dependencies and vite/dist chunks, so it's not costing unnecessary parsing/magic-string work.

@hi-ogawa hi-ogawa changed the title fix(vitest): simplify hoist transform check regex to avoid regex DoS fix(vitest): simplify hoist transform check regex to avoid expensive regex match Jan 16, 2024
@sheremet-va sheremet-va merged commit df0db6a into vitest-dev:main Jan 16, 2024
@hi-ogawa hi-ogawa deleted the fix-regexpHoistable-dos branch January 16, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants