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.

Sign up for GitHub

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

Jump to bottom

[20.14.0][fs] recursive watch on Linux crashes on .close() #53350

Closed
AviVahl opened this issue Jun 5, 2024 · 7 comments · Fixed by #53452
Closed

[20.14.0][fs] recursive watch on Linux crashes on .close() #53350

AviVahl opened this issue Jun 5, 2024 · 7 comments · Fixed by #53452
Labels
watch-mode Issues and PRs related to watch mode

Comments

@AviVahl
Copy link

AviVahl commented Jun 5, 2024 •
edited by VoltrexKeyva
Loading

Version

20.14.0

Platform

Linux fedora 6.8.11-300.fc40.x86_64 #1 SMP PREEMPT_DYNAMIC Mon May 27 14:53:33 UTC 2024 x86_64 GNU/Linux

Subsystem

internal/fs/recursive_watch

What steps will reproduce the bug?

  1. Create a folder with watch.mjs:
import { watch } from "node:fs";

const watcher = watch(import.meta.dirname, { recursive: true });
watcher.on("change", (eventType, filename) => {
  // console.log(eventType, filename);
});
watcher.on("error", (e) => {
  console.error(e);
});

console.log("Watching for changes...");
  1. git init in folder
  2. node watch.mjs
  3. in the same folder, run the following:
for run in {1..100}; do git add watch.mjs && git rm --cached watch.mjs; done

the above command stages and unstages watch.mjs 100 times. you should see watch errors

Error: ENOENT: no such file or directory, stat '/home/avi/projects/watch-crash/.git/index.lock'
    at statSync (node:fs:1658:25)
    at #watchFile (node:internal/fs/recursive_watch:152:28)
    at #watchFolder (node:internal/fs/recursive_watch:129:26)
    at FSWatcher.<anonymous> (node:internal/fs/recursive_watch:184:26)
    at FSWatcher.emit (node:events:519:28)
    at FSWatcher._handle.onchange (node:internal/fs/watchers:215:12)

and

Error: ENOENT: no such file or directory, watch '/home/avi/projects/watch-crash/.git/index.lock'
    at FSWatcher.<computed> (node:internal/fs/watchers:247:19)
    at watch (node:fs:2469:36)
    at #watchFile (node:internal/fs/recursive_watch:156:21)
    at #watchFolder (node:internal/fs/recursive_watch:129:26)
    at FSWatcher.<anonymous> (node:internal/fs/recursive_watch:184:26)
    at FSWatcher.emit (node:events:519:28)
    at FSWatcher._handle.onchange (node:internal/fs/watchers:215:12)

This is because git saves the lock, and removes it immediately.
Sometimes this statSync call fails (recursive_watch:152):

{
      const existingStat = statSync(file);
      this.#files.set(file, existingStat);
    }

which could have been avoided by using throwIfNoEntry: false and checking the return value.

the real race happens when the above statSync call succeeds, and then the watch() call (two lines down) fails because the file just got deleted. This can be seen in the second stack trace above. In this scenario, this.#files will be populated with the stats object, but this.#watchers won't have a matching watcher registered in the map.

When calling close() on watcher after the second scenario happened, the following loop in close will fail:

    for (const file of this.#files.keys()) {
      this.#watchers.get(file).close();
      this.#watchers.delete(file);
    }

as this.#watchers.get(file).close(); will fail when trying to call close() on undefined (watch() call blew up, so nothing in this.#watchers for file).

the error looks like:

TypeError: Cannot read properties of undefined (reading 'close')
      at FSWatcher.close (node:internal/fs/recursive_watch:86:31)

How often does it reproduce? Is there a required condition?

Pretty consistently.

What is the expected behavior? Why is that the expected behavior?

Don't crash on close().
When querying whether a node exists or not (statSync), use throwIfNoEntry to avoid generated errors.

What do you see instead?

calling close() crashes the process

Additional information

@mcollina you might be interested. looks like your cup of tea.
@marco-ippolito marco-ippolito added the watch-mode Issues and PRs related to watch mode label Jun 5, 2024
@jakecastelli
Copy link
Contributor

I am on macOS aarch 64 with node v20.13.1 - I didn't see the same error but I got fatal: pathspec 'indx.mjs' did not match any files when do for run in {1..100}; do git add watch.mjs && git rm --cached watch.mjs; done, I assume I will need to test this on Linux.

Also I added console.count('file has been changed') in the on('change') and I got file has been changed: 207 207 times.

@AviVahl
Copy link
Author

AviVahl commented Jun 6, 2024

The for run in {1..100}; do git add watch.mjs && git rm --cached watch.mjs; done is just a bash one-liner to git stage and unstage watch.mjs 100 times really quickly. This causes the .lock file to be created and deleted quickly many times.

@jakecastelli
Copy link
Contributor

jakecastelli commented Jun 6, 2024 •
edited
Loading

yes, I did run it as a bash one-liner, and that's what I got from the tty fatal: pathspec 'index.mjs' did not match any files

edit: I didn't change the file name in the bash one liner script to index.mjs, after I changed to index.mjs and it works, but it didn't crash on Mac.

@AviVahl
Copy link
Author

AviVahl commented Jun 7, 2024

Not sure where the indx.mjs came from. file has to be named watch.mjs for the steps to work.
Alternatively, I've also reproduce this, and additional full crashes (unhandled exceptions) using:
for run in {1..100}; do git init && rm -rf .git; done

above will initialize a git repo in the current directory and remove it right away 100 times.

make sure you do it all in a new empty directory.

@jakecastelli
Copy link
Contributor

Not sure where the indx.mjs came from

Sorry for the confusion, I renamed watch.mjs to index.mjs on my end, but apart from that everything else should be the same.

I tested for run in {1..100}; do git init && rm -rf .git; done, there is no error this time, but I didn't reproduce the crash, probably because fs implementation in libuv for mac and linux is different.

I tested on version node v20.14 mac m1.

Will see what other people reproduce on this one.

@AviVahl
Copy link
Author

AviVahl commented Jun 7, 2024

I believe It has to be Linux to be using the internal recursive_watch.js (and see the issue)

@mcollina mcollina mentioned this issue Jun 14, 2024
Merged
@mcollina
Copy link
Member

Here is a fix: #53452

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
watch-mode Issues and PRs related to watch mode
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fs: do not crash if the watched file is removed while setting up watch mcollina/node
5 participants
@mcollina @AviVahl @marco-ippolito @jakecastelli and others