fix(haste-map): multiple resolutions on failure #12420
Conversation
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) { | |||
There was a problem hiding this comment.
I think this makes sense. is the error you're seeing mitigated by this change, or is it an educated guess?
There was a problem hiding this comment.
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 matchesNote 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 matchesAll runs after that contain the correct amount of files.
|
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. |
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.