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

watch: fix null fileName on windows systems #49891

Merged
merged 1 commit into from
Dec 28, 2023

Conversation

vnc5
Copy link
Contributor

@vnc5 vnc5 commented Sep 27, 2023

This fixes a Node.js crash when running on Windows with --watch.
The issue is that fs.watch() sometimes returns null for the filename as described in the official Node.js documentation.

The thrown error:

node:internal/validators:162
    throw new ERR_INVALID_ARG_TYPE(name, 'string', value);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "paths[1]" argument must be of type string. Received null
    at __node_internal_captureLargerStackTrace (node:internal/errors:496:5)
    at new NodeError (node:internal/errors:405:5)
    at validateString (node:internal/validators:162:11)
    at resolve (node:path:171:9)
    at FSWatcher.<anonymous> (node:internal/watch_mode/files_watcher:99:30)
    at FSWatcher.emit (node:events:514:28)
    at FSWatcher._handle.onchange (node:internal/fs/watchers:215:12) {
  code: 'ERR_INVALID_ARG_TYPE'
}

Node.js v20.5.1

There is also a Stack Overflow question related to this issue.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 27, 2023
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 29, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Why is fileName null in this case? if it is do we want to fire #onChange ?

@vnc5
Copy link
Contributor Author

vnc5 commented Sep 29, 2023

I'm not quite sure what the correct interpretation of null would be and what action to take.

@benjamingr
Copy link
Member

@nodejs/fs @bnoordhuis do you happen to know? (why/when null can appear as fileName)

@bnoordhuis
Copy link
Member

why/when null can appear as fileName

That can happen when the ReadDirectoryChanges IOCP request doesn't complete properly. The NT kernel has this half-way state where the request doesn't fail outright but neither does it produce a valid file change event.

Libuv reports it as a UV_CHANGE event with a NULL path because presumably something changed even if we don't know what.

I suppose libuv could pass the original path (the path it's been told to watch) although I'm not completely sure that's 100% harmless.

@@ -98,7 +98,7 @@ class FilesWatcher extends EventEmitter {
}
const watcher = watch(path, { recursive, signal: this.#signal });
watcher.on('change', (eventType, fileName) => this
.#onChange(recursive ? resolve(path, fileName) : path));
.#onChange(recursive ? resolve(path, fileName ?? '') : path));
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment to here referencing/mentioning @bnoordhuis's comments? It's important to not lose any knowledge we have.

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 28, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 28, 2023
@nodejs-github-bot nodejs-github-bot merged commit 9db4bf4 into nodejs:main Dec 28, 2023
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 9db4bf4

@anonrig
Copy link
Member

anonrig commented Dec 28, 2023

We shouldn't have merged this while having an open/pending pull request review comment.

@lpinca
Copy link
Member

lpinca commented Dec 28, 2023

Was it a blocker? We can add the comment now without waiting further. This was open for 3 months.

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
PR-URL: #49891
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #49891
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #49891
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants