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

fs.isWatchAvailable() [capabilities check for fs.watch()] #29894

Closed
ThePrez opened this issue Oct 9, 2019 · 9 comments
Closed

fs.isWatchAvailable() [capabilities check for fs.watch()] #29894

ThePrez opened this issue Oct 9, 2019 · 9 comments
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding.

Comments

@ThePrez
Copy link
Contributor

ThePrez commented Oct 9, 2019

Is your feature request related to a problem? Please describe.
The fs.watch() File System method is handy and useful. Application and module developers are incented to use it for its advantages over fs.watchFile(). However, it is inconsistent across platforms, and the documentation states that it "is unavailable in some situations."

Application developers lack a good way of knowing whether the API is even available, making it diffucult to write portable JavaScript code. The documentation does a nice job of hitting some of the platforms and conditions, but it would be tedious for a developer to write logic around this, not to mention managing periodic updates as new conditions/platforms are added.

Describe the solution you'd like
I'd like for a new method to be available in fs to check whether watch() is available. That way, portable code can be written that "falls back" to fs.watchFile() if feasible.

Not only would this enable portability, it would enable OS-specific conditions to be checked within the Node.js implementation. For example, the AIX implementation could check whether AHAFS is enabled.

Even further, if the new isWatchAvailable() method could take a directory as a parameter, then its implementation could also check whether fs.watch() would function on objects within that directory (and therefore say "no" for SMB shares, etc).

Describe alternatives you've considered
Implementations could throw a specific Exception/Error if the platform doesn't support fs.watch(). However, it seems like the behavior is completely unspecified when the function is not available.

@Trott Trott added feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. labels Oct 9, 2019
@Trott
Copy link
Member

Trott commented Oct 9, 2019

@nodejs/fs

@bnoordhuis bnoordhuis added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Oct 9, 2019
@bnoordhuis
Copy link
Member

Putting TOCTOU issues aside... It would need to be implemented in libuv first but there will be situations where libuv can't determine whether file watching would work, so you'd either have to accept false positives or false negatives. Doesn't seem that useful to me.

@saghul
Copy link
Member

saghul commented Oct 9, 2019

Rather than add a way to check (which opens the door for TOCTOU, as Ben pointed out) I think a better approach would be to try to fail hard if it doesn't work. That way you could catch the exception, and swtich to an alternative method.

Detecting all those conditions, is, however, not easy AFAIK.

@ThePrez
Copy link
Contributor Author

ThePrez commented Oct 9, 2019

At a minimum, I would suggest some definition of what is to happen when fs.watch() is unavailable, hopefully in a way that can allow programmers to "fall back." As it is currently written and documented, it is virtually impossible to write portable code that utilizes this functionality.

A specific runtime Exception at the point of invocation would suffice, while also addressing TOCTOU concerns.

@bnoordhuis
Copy link
Member

It's never unavailable, sometimes the OS simply just doesn't reports events. How do you suggest libuv deals with that?

Whitelisting/blacklisting based on the fs type isn't good enough because of false positives/negatives (and also suffers from TOCTOU issues.)

@ThePrez
Copy link
Contributor Author

ThePrez commented Oct 9, 2019

It's never unavailable, sometimes the OS simply just doesn't reports events. How do you suggest libuv deals with that?

Whitelisting/blacklisting based on the fs type isn't good enough because of false positives/negatives (and also suffers from TOCTOU issues.)

Since it's virtually impossible to avoid false positives/negatives, you are absolutely right that the notion of writing truly portable code with conditional use of fs.watch() is inherently impossible (which is OK).

After considering your points, it's clear that the initial enhancement request is not a good idea. Still, there are times when the implementation will know for certain that the OS won't report events. It'd be (arguably) nice to have some indicator in these cases (and my suggestion would be a hard fail with a thrown Error to minimize TOCTOU problems). For instance, I'd be proposing the addition of the following to the documentation:

In instances where it is known that the OS cannot report events for the given file, this method may throw an Error upon invocation.

Also, for the sake of complete disclosure: I have a specific vested interest since I'm on the platform team for IBM i (@libuv/ibmi). There are scenarios where we know watching won't work and we'd like the option of throwing an Error accordingly (maybe we already can, but the doc is not clear whether this is allowed).

@richardlau
Copy link
Member

I'm fairly certain on AIX an error is thrown if watch() is unavailable. It's the reason for this catch block in the report tests:

// Watching files should result in fs_event/fs_poll uv handles.
let watcher;
try {
watcher = fs.watch(__filename);
} catch {
// fs.watch() unavailable
}
fs.watchFile(__filename, () => {});

@ThePrez
Copy link
Contributor Author

ThePrez commented Oct 9, 2019

I'm fairly certain on AIX an error is thrown if watch() is unavailable. It's the reason for this catch block in the report tests:

// Watching files should result in fs_event/fs_poll uv handles.
let watcher;
try {
watcher = fs.watch(__filename);
} catch {
// fs.watch() unavailable
}
fs.watchFile(__filename, () => {});

Thanks! I didn't catch that!
So, I think my enhancement proposal boils down to a documentation change to clearly state that an error may be thrown.

@ThePrez
Copy link
Contributor Author

ThePrez commented Apr 6, 2020

Thanks all for handling!

BethGriggs pushed a commit that referenced this issue Apr 7, 2020
Fixes: #29894

PR-URL: #32513
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this issue Apr 12, 2020
Fixes: #29894

PR-URL: #32513
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this issue Apr 22, 2020
Fixes: #29894

PR-URL: #32513
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants