Skip to content

Conversation

@pipobscure
Copy link
Contributor

@pipobscure pipobscure commented May 28, 2025

This addresses a bug in fs.watch when used as an AsyncIterator.

The issue is that when consuming the AsyncIteractor returned by fs.watch it yields a value. When using that value and in turn awaiting an asynchrounous action any events happening in the meantime will go missing. The reason is that between exiting the watch function by yielding and reentering it through the next round, the promise inside watch is already resolved. So any events generated will be duplicate resolutions of that promise and therefore ignored.

for await (let found of fs.watch('my dir')) {
  console.log(found);
  await sleep(10000); // any events happening during these 10seconds will  be missed;
}

More reaslistically than this minimal example is when the found files are actually read via await fs.readFile. Then there will be a lag. If the file events happening are happening close together, then the second one will be missed.

To fix this issue I added a queue to the watch function that new file events get pushed onto. The promise is no longer resolved with a value, but is simply the gate gating whether or not there are any events in the queue. The iterator awaits the promise and then yields the items from the queue so long as there are any. When the queue is empty and the watch is still running, then a new promise is created and awaited upon. This whould eliminate the problem entirely and one can now go asynchronous in the loop as long as one wants without missing events.

Verified that the added test fails with v24.0.0 and passes after the fix.

Based on feedback the queuing was made configurable with the maxQueue (default 2048) option determining the maximum size of the queue and the overflow option deciding to either ignore the issue or throw an Error (default: 'ignore'). To effectively get back the previous behavior one would have to pass { maxQueue: 1, overflow: 'ignore' }.

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels May 28, 2025
@pipobscure pipobscure marked this pull request as ready for review May 28, 2025 22:49
@codecov
Copy link

codecov bot commented May 28, 2025

Codecov Report

Attention: Patch coverage is 72.50000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 90.15%. Comparing base (40d8983) to head (218d461).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/fs/watchers.js 71.79% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58490      +/-   ##
==========================================
- Coverage   90.16%   90.15%   -0.01%     
==========================================
  Files         636      636              
  Lines      187891   187923      +32     
  Branches    36884    36882       -2     
==========================================
+ Hits       169408   169429      +21     
- Misses      11246    11247       +1     
- Partials     7237     7247      +10     
Files with missing lines Coverage Δ
lib/internal/errors.js 97.48% <100.00%> (+<0.01%) ⬆️
lib/internal/fs/watchers.js 85.16% <71.79%> (-0.63%) ⬇️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pipobscure pipobscure marked this pull request as draft May 29, 2025 10:33
@pipobscure pipobscure force-pushed the fsbug branch 2 times, most recently from 63d31e7 to 3512480 Compare May 29, 2025 11:23
@pipobscure pipobscure marked this pull request as ready for review May 29, 2025 11:23
buhski

This comment was marked as spam.

@pipobscure pipobscure force-pushed the fsbug branch 8 times, most recently from 16c2f33 to d5d6d5d Compare June 4, 2025 16:08
@joyeecheung
Copy link
Member

This almost LGTM % the suggestions. Though I'd appreciate if others from @nodejs/fs or maybe @jasnell @benjamingr can take a look and see if the new options look good.

Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

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

Other than the option values, this LGTM

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM % missing updates to the docs

@pipobscure
Copy link
Contributor Author

Thanks for all the help, guidance & input @joyeecheung & @Ethan-Arrowood ❤️

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 6, 2025
@joyeecheung joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 9, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 10, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 10, 2025
@nodejs-github-bot nodejs-github-bot merged commit 5f7dbf4 into nodejs:main Jun 10, 2025
60 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 5f7dbf4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants