fs: allow correct handling of burst in fs-events with AsyncIterator by pipobscure · Pull Request #58490 · nodejs/node · GitHub

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.

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.

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.

Other than the option values, this LGTM

LGTM % missing updates to the docs

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

CI: https://ci.nodejs.org/job/node-test-pull-request/67350/

CI: https://ci.nodejs.org/job/node-test-pull-request/67365/

Landed in 5f7dbf4

PR-URL: nodejs#58490 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #58490 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Notable changes: doc: * add islandryu to collaborators (Shima Ryuhei) #58714 fs: * (SEMVER-MINOR) allow correct handling of burst in fs-events with AsyncIterator (Philipp Dunkel) #58490 module: * (SEMVER-MINOR) remove experimental warning from type stripping (Marco Ippolito) #58643 test_runner: * (SEMVER-MINOR) support object property mocking (Idan Goshen) #58438 url: * (SEMVER-MINOR) add fileURLToPathBuffer API (James M Snell) #58700 PR-URL: #58727

Notable changes: doc: * add islandryu to collaborators (Shima Ryuhei) #58714 fs: * (SEMVER-MINOR) allow correct handling of burst in fs-events with AsyncIterator (Philipp Dunkel) #58490 module: * (SEMVER-MINOR) remove experimental warning from type stripping (Marco Ippolito) #58643 test_runner: * (SEMVER-MINOR) support object property mocking (Idan Goshen) #58438 url: * (SEMVER-MINOR) add fileURLToPathBuffer API (James M Snell) #58700 PR-URL: #58804

Notable changes: doc: * add islandryu to collaborators (Shima Ryuhei) #58714 fs: * (SEMVER-MINOR) allow correct handling of burst in fs-events with AsyncIterator (Philipp Dunkel) #58490 module: * (SEMVER-MINOR) remove experimental warning from type stripping (Marco Ippolito) #58643 test: * fix test-timeout-flag after revert of auto subtest wait (Pietro Marchini) #58282 test_runner: * Revert "test_runner: remove promises returned by t.test() (Romain Menke) #58282 * Revert "test_runner: remove promises returned by test() (Romain Menke) #58282 * Revert "test_runner: automatically wait for subtests to finish (Romain Menke) #58282 * (SEMVER-MINOR) support object property mocking (Idan Goshen) #58438 url: * (SEMVER-MINOR) add fileURLToPathBuffer API (James M Snell) #58700 PR-URL: #58813

Notable changes: doc: * add islandryu to collaborators (Shima Ryuhei) #58714 fs: * (SEMVER-MINOR) allow correct handling of burst in fs-events with AsyncIterator (Philipp Dunkel) #58490 module: * (SEMVER-MINOR) remove experimental warning from type stripping (Marco Ippolito) #58643 test: * fix test-timeout-flag after revert of auto subtest wait (Pietro Marchini) #58282 test_runner: * (SEMVER-MINOR) support object property mocking (Idan Goshen) #58438 url: * (SEMVER-MINOR) add fileURLToPathBuffer API (James M Snell) #58700 PR-URL: #58813

PR-URL: #58490 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Notable changes: deps: * (SEMVER-MINOR) update amaro to 1.1.0 (Node.js GitHub Bot) #56350 doc: * add islandryu to collaborators (Shima Ryuhei) #58714 esm: * (SEMVER-MINOR) implement `import.meta.main` (Joe) #57804 fs: * (SEMVER-MINOR) allow correct handling of burst in fs-events with AsyncIterator (Philipp Dunkel) #58490 module: * (SEMVER-MINOR) remove experimental warning from type stripping (Marco Ippolito) #56350 * (SEMVER-MINOR) unflag `--experimental-strip-types` (Marco Ippolito) #56350 permission: * (SEMVER-MINOR) propagate permission model flags on spawn (Rafael Gonzaga) #58853 sqlite: * (SEMVER-MINOR) add support for `readBigInts` option in db connection level (Miguel Marcondes Filho) #58697 src,permission: * (SEMVER-MINOR) add support to `permission.has(addon)` (Rafael Gonzaga) #58951 url: * (SEMVER-MINOR) add `fileURLToPathBuffer` API (James M Snell) #58700 watch: * (SEMVER-MINOR) add `--watch-kill-signal` flag (Dario Piotrowicz) #58719 worker: * (SEMVER-MINOR) make `Worker` async disposable (James M Snell) #58385 PR-URL: #59256 Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Notable changes: deps: * (SEMVER-MINOR) update amaro to 1.1.0 (Node.js GitHub Bot) nodejs#56350 doc: * add islandryu to collaborators (Shima Ryuhei) nodejs#58714 esm: * (SEMVER-MINOR) implement `import.meta.main` (Joe) nodejs#57804 fs: * (SEMVER-MINOR) allow correct handling of burst in fs-events with AsyncIterator (Philipp Dunkel) nodejs#58490 module: * (SEMVER-MINOR) remove experimental warning from type stripping (Marco Ippolito) nodejs#56350 * (SEMVER-MINOR) unflag `--experimental-strip-types` (Marco Ippolito) nodejs#56350 permission: * (SEMVER-MINOR) propagate permission model flags on spawn (Rafael Gonzaga) nodejs#58853 sqlite: * (SEMVER-MINOR) add support for `readBigInts` option in db connection level (Miguel Marcondes Filho) nodejs#58697 src,permission: * (SEMVER-MINOR) add support to `permission.has(addon)` (Rafael Gonzaga) nodejs#58951 url: * (SEMVER-MINOR) add `fileURLToPathBuffer` API (James M Snell) nodejs#58700 watch: * (SEMVER-MINOR) add `--watch-kill-signal` flag (Dario Piotrowicz) nodejs#58719 worker: * (SEMVER-MINOR) make `Worker` async disposable (James M Snell) nodejs#58385 PR-URL: nodejs#59256 Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

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 marked this pull request as ready for review May 28, 2025 22:49

pipobscure marked this pull request as draft May 29, 2025 10:33

pipobscure force-pushed the fsbug branch 2 times, most recently from 63d31e7 to 3512480 Compare May 29, 2025 11:23

pipobscure marked this pull request as ready for review May 29, 2025 11:23

pipobscure force-pushed the fsbug branch from 3512480 to a52c929 Compare May 29, 2025 11:27

joyeecheung reviewed May 29, 2025

View reviewed changes

Comment thread lib/internal/fs/watchers.js Outdated

joyeecheung reviewed May 30, 2025

View reviewed changes

Comment thread lib/internal/fs/watchers.js Outdated

Comment thread lib/internal/fs/watchers.js Outdated

Comment thread test/parallel/test-fs-promises-watch-iterator.js Outdated

This comment was marked as spam.

Sign in to view

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

joyeecheung reviewed Jun 4, 2025

View reviewed changes

joyeecheung reviewed Jun 6, 2025

View reviewed changes

Comment thread lib/internal/fs/watchers.js Outdated

Comment thread lib/internal/errors.js Outdated

Comment thread doc/api/errors.md Outdated

Comment thread test/parallel/test-fs-promises-watch.js Outdated

Comment thread doc/api/fs.md Outdated

Ethan-Arrowood reviewed Jun 6, 2025

View reviewed changes

Comment thread doc/api/fs.md Outdated

joyeecheung reviewed Jun 6, 2025

View reviewed changes

Comment thread test/parallel/test-fs-promises-watch.js Outdated

joyeecheung approved these changes Jun 6, 2025

View reviewed changes

Comment thread doc/api/fs.md Outdated

Ethan-Arrowood approved these changes Jun 6, 2025

View reviewed changes

joyeecheung approved these changes Jun 6, 2025

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 6, 2025

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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 9, 2025

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 10, 2025

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 merged commit 5f7dbf4 into nodejs:main Jun 10, 2025

github-actions Bot mentioned this pull request Jun 16, 2025

2025-06-17, Version 24.3.0 (Current) #58727

Closed

github-actions Bot mentioned this pull request Jun 23, 2025

2025-06-24, Version 24.3.0 (Current) #58804

Closed

github-actions Bot mentioned this pull request Jun 24, 2025

2025-06-24, Version 24.3.0 (Current) #58813

Merged

github-actions Bot mentioned this pull request Jul 28, 2025

2025-07-29, Version 22.18.0 'Jod' (LTS) #59256

Merged

fs: allow correct handling of burst in fs-events with AsyncIterator by pipobscure · Pull Request #58490 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: allow correct handling of burst in fs-events with AsyncIterator - #58490

fs: allow correct handling of burst in fs-events with AsyncIterator#58490nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom pipobscure:fsbugpipobscure/node:fsbugCopy head branch name to clipboard

Conversation

pipobscure commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joyeecheung commented Jun 6, 2025

Uh oh!

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pipobscure commented Jun 6, 2025

Uh oh!

nodejs-github-bot commented Jun 9, 2025

Uh oh!

nodejs-github-bot commented Jun 9, 2025

Uh oh!

nodejs-github-bot commented Jun 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fs: allow correct handling of burst in fs-events with AsyncIterator#58490
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
pipobscure:fsbug

pipobscure commented May 28, 2025 •
edited
Loading

codecov Bot commented May 28, 2025 •
edited
Loading