fsevents: detect watched directory removal #4376
Conversation
|
Not completely sure this won't break some other stuff @vtjnash ? |
santigimeno
force-pushed
the
santi/fs_events_fix
branch
from
fe00e44
to
eed8b52
Compare
There was a problem hiding this comment.
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`.
santigimeno
force-pushed
the
santi/fs_events_fix
branch
from
3e0ee85
to
0ea5398
Compare
santigimeno
changed the title
|
Updated fixing also the functionality on Windows. PTAL |
|
The USBAN failure seems unrelated to this change |
santigimeno
force-pushed
the
santi/fs_events_fix
branch
from
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.
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.
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.
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.
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.
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.
I think I found a solution by retrieving FileStandardInfo from the handle. PTAL
There was a problem hiding this comment.
@bnoordhuis @vtjnash thoughts?
There was a problem hiding this comment.
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
windowsandmacos.Fixes: nodejs/node#52055