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

fix(haste-map): multiple resolutions on failure #12420

Merged
merged 2 commits into from
Feb 18, 2022
Merged

fix(haste-map): multiple resolutions on failure #12420

merged 2 commits into from
Feb 18, 2022

Conversation

oliversalzburg
Copy link
Contributor

Summary

When hitting any error, the result callback would be invoked with an intermediate result. There could still be active searches, that later invoke the callback with the final result. This multi-resolution introduces race conditions and corrupts the haste map cache.

Test plan

Reproducing these issues is very difficult, as they require certain race conditions to be met. Reviewing this code should clearly show that the current approach is faulty. If this is the correct solution, is debateable.

When hitting any error, the result callback would be invoked with an intermediate result. There could still be active searches, that later invoke the callback with the final result. This multi-resolution introduces race conditions and corrupts the haste map cache.
@@ -71,7 +71,9 @@ function find(
fs.readdir(directory, {withFileTypes: true}, (err, entries) => {
activeCalls--;
if (err) {
callback(result);
if (activeCalls === 0) {
Copy link
Member

@SimenB SimenB Feb 18, 2022

Choose a reason for hiding this comment

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

I think this makes sense. is the error you're seeing mitigated by this change, or is it an educated guess?

Copy link
Contributor Author

@oliversalzburg oliversalzburg Feb 18, 2022

Choose a reason for hiding this comment

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

It is mitigated. I was running Jest in a debugger in our monorepo to understand what was going on and this change reliably restored operations to give stable results.

Here is an example of 2 consecutive runs in our current monorepo with the code as-is:

$ yarn jest thisdoesnotexist
No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In /home/oliver/fairmanager/core-web3
  6541 files checked.
  testMatch:  - 0 matches
  testPathIgnorePatterns: /node_modules/ - 6541 matches
  testRegex: (/__tests__/.*|(\.|/)(test|spec))\.tsx?$ - 172 matches
Pattern: thisdoesnotexist - 0 matches

$ yarn jest thisdoesnotexist
No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In /home/oliver/fairmanager/core-web3
  5436 files checked.
  testMatch:  - 0 matches
  testPathIgnorePatterns: /node_modules/ - 5436 matches
  testRegex: (/__tests__/.*|(\.|/)(test|spec))\.tsx?$ - 172 matches
Pattern: thisdoesnotexist - 0 matches

Note that the number of files checked decreased. This is the result of the map being generated, hitting the error, and then persisting the partial map to disk. Operations then continue, the search finally completes and the full set of files is pushed into the map.
When I run the second time, I only get the files that were in the cache from the first run.

After making the change suggested in this PR, the file count will restore to the original, full count:

$ vi node_modules/jest-haste-map/build/crawlers/node.js

$ yarn jest thisdoesnotexist
No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In /home/oliver/fairmanager/core-web3
  6541 files checked.
  testMatch:  - 0 matches
  testPathIgnorePatterns: /node_modules/ - 6541 matches
  testRegex: (/__tests__/.*|(\.|/)(test|spec))\.tsx?$ - 172 matches
Pattern: thisdoesnotexist - 0 matches

All runs after that contain the correct amount of files.

Copy link
Member

Choose a reason for hiding this comment

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

cool, good enough for me! 😀

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

@SimenB SimenB merged commit f3c48b5 into jestjs:main Feb 18, 2022
@oliversalzburg oliversalzburg deleted the fix/haste-generation branch February 18, 2022 10:04
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants