-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fsevents: detect watched directory removal #4376
base: v1.x
Are you sure you want to change the base?
Conversation
Not completely sure this won't break some other stuff @vtjnash ? |
fe00e44
to
eed8b52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there maybe is (or was) a case where it watched a whole directory instead of a file, so it was doing this in addition to checking the prefix match? I can't think of any other reason that check would be there, or any reason that check should be kept there
The UNIX build errors can be fixed by including |
Which was broken both in `windows` and `macos`.
3e0ee85
to
0ea5398
Compare
Updated fixing also the functionality on Windows. PTAL |
The USBAN failure seems unrelated to this change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested it locally on Windows and it works.
2e551e1
to
756fe04
Compare
src/win/fs-event.c
Outdated
if (err == ERROR_ACCESS_DENIED && | ||
handle->dirw && | ||
GetFileAttributesW(handle->dirw) == INVALID_FILE_ATTRIBUTES) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style:
if (err == ERROR_ACCESS_DENIED && | |
handle->dirw && | |
GetFileAttributesW(handle->dirw) == INVALID_FILE_ATTRIBUTES) { | |
if (err == ERROR_ACCESS_DENIED && | |
handle->dirw != NULL && | |
GetFileAttributesW(handle->dirw) == INVALID_FILE_ATTRIBUTES) { |
Substance: since the check is path-based, not handle-based, isn't this basically a race condition? The directory may have been recreated, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Substance: since the check is path-based, not handle-based, isn't this basically a race condition? The directory may have been recreated, etc.
Agreed. The problem is that checking through the handle I wasn't able to detect whether the directory was already deleted or not (GetFileInformationByHandle()
would always succeed) so unless there's another way to detect it maybe this solution is better than what we already have?
/cc @vtjnash as he might know of a possible solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you uv_stat both the current path and the handle, you could check if it is the same inode (if you still have permissions to access it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that checking GetFileAttributesW()
for the current path already returns INVALID_FILE_ATTRIBUTES
so it doesn't seem this approach would work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also try CompareObjectHandles to directly see if the new handle is the same "underlying object" as the new one, if the open succeeds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found a solution by retrieving FileStandardInfo
from the handle. PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnoordhuis @vtjnash thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds interesting, but a folder could still end up not deleted (because it the user clears the flag later) and since a folder now can end up deleted without setting that flag (by requesting posix semantics for the delete, or by moving it), it would be good to test this with #4318 at least to make sure it works correctly
Which was broken both in
windows
andmacos
.Fixes: nodejs/node#52055