Skip to content

fsevents: detect watched directory removal#4376

Merged
santigimeno merged 1 commit intolibuv:v1.xfrom
santigimeno:santi/fs_events_fix
Jul 29, 2024
Merged

fsevents: detect watched directory removal#4376
santigimeno merged 1 commit intolibuv:v1.xfrom
santigimeno:santi/fs_events_fix

Conversation

@santigimeno
Copy link
Copy Markdown
Member

@santigimeno santigimeno commented Mar 31, 2024 •
edited
Loading

Which was broken both in windows and macos.

Fixes: nodejs/node#52055

@santigimeno
Copy link
Copy Markdown
Member Author

santigimeno commented Mar 31, 2024

Not completely sure this won't break some other stuff @vtjnash ?

@santigimeno santigimeno mentioned this pull request Mar 31, 2024
Closed
@santigimeno santigimeno force-pushed the santi/fs_events_fix branch from fe00e44 to eed8b52 Compare March 31, 2024 13:33
vtjnash
vtjnash approved these changes Mar 31, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@vtjnash vtjnash left a 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

@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Mar 31, 2024

The UNIX build errors can be fixed by including <unistd.h>. On Windows, I'm not sure.

@santigimeno santigimeno force-pushed the santi/fs_events_fix branch from 3e0ee85 to 0ea5398 Compare April 1, 2024 09:15
@santigimeno santigimeno changed the title fsevents,macos: detect watched directory removal fsevents: detect watched directory removal Apr 1, 2024
@santigimeno
Copy link
Copy Markdown
Member Author

santigimeno commented Apr 1, 2024

Updated fixing also the functionality on Windows. PTAL

@santigimeno
Copy link
Copy Markdown
Member Author

santigimeno commented Apr 1, 2024

The USBAN failure seems unrelated to this change

huseyinacacak-janea
huseyinacacak-janea approved these changes Apr 3, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@huseyinacacak-janea huseyinacacak-janea left a 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.

saghul
saghul reviewed Apr 6, 2024
View reviewed changes
Comment thread test/test-fs-event.c Outdated
@santigimeno santigimeno force-pushed the santi/fs_events_fix branch from 2e551e1 to 756fe04 Compare April 6, 2024 21:11
bnoordhuis
bnoordhuis reviewed Apr 14, 2024
View reviewed changes
Comment thread src/win/fs-event.c Outdated
@santigimeno santigimeno mentioned this pull request Jul 26, 2024
Closed
@santigimeno santigimeno force-pushed the santi/fs_events_fix branch 2 times, most recently from 5fafce2 to c4e94e3 Compare July 26, 2024 15:41
@santigimeno
fsevents: detect watched directory removal
9ecf830 
Which was broken both in `windows` and `macos`.
@santigimeno santigimeno force-pushed the santi/fs_events_fix branch from e2f05ce to 9ecf830 Compare July 29, 2024 22:47
@santigimeno santigimeno merged commit badecdc into libuv:v1.x Jul 29, 2024
@santigimeno santigimeno deleted the santi/fs_events_fix branch July 29, 2024 22:59
heehaw12345
heehaw12345 approved these changes Aug 6, 2024
View reviewed changes
Copy link
Copy Markdown

@heehaw12345 heehaw12345 left a comment

Choose a reason for hiding this comment

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

  • [x]

@Tofandel
Copy link
Copy Markdown

Tofandel commented Dec 16, 2024 •
edited
Loading

This PR seems to introduce a (big) memory leak in the win/fs events path

In

libuv/src/win/util.c

Line 1031 in 3d78d12

return uv_utf16_to_wtf8(utf16, utf16len, utf8, &utf8_len);
doesn't the original filename need to be freed?

@saghul
Copy link
Copy Markdown
Member

saghul commented Dec 16, 2024

No, handle->dirw is freed in uv_fs_event_stop:

libuv/src/win/fs-event.c

Line 390 in 3d78d12

if (handle->dirw) {

Are you observing a leak or just browsing through the code?

@Tofandel
Copy link
Copy Markdown

Tofandel commented Dec 16, 2024

We are oberving a leak which starts only after this commit (nuxt/devtools#761), it might be that this fix now makes it that it goes into the code path that #1263 fixes

saghul added a commit that referenced this pull request Dec 16, 2024
@saghul
win: fix leak processing fs event
8fc1a01 
Fixes: #4376 (comment)
@saghul saghul mentioned this pull request Dec 16, 2024
Merged
@saghul
Copy link
Copy Markdown
Member

saghul commented Dec 16, 2024

Ah, got it! PR: #4656

The converted filename (not the OG) needs to be freed afterwards indeed.

saghul added a commit that referenced this pull request Dec 16, 2024
@saghul
win: fix leak processing fs event
ec5a4b5 
Fixes: #4376 (comment)
@xiaoxiaojx xiaoxiaojx mentioned this pull request May 14, 2025
Merged
alexander-akait pushed a commit to webpack/watchpack that referenced this pull request May 16, 2025
@xiaoxiaojx
fix: ignore listening for the directory itself to be deleted (#251)
919c666 
After Node.js v22, fs.watch(dir) and deleting a dir will trigger the
rename change event.
Here we just ignore it and keep the same behavior as before v22
libuv/libuv#4376
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@saghul saghul saghul approved these changes

@vtjnash vtjnash vtjnash approved these changes

+2 more reviewers

@heehaw12345 heehaw12345 heehaw12345 approved these changes

@huseyinacacak-janea huseyinacacak-janea huseyinacacak-janea approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

macOS: fs.watch does not report delete of watched folder

7 participants

@santigimeno @bnoordhuis @Tofandel @saghul @vtjnash @heehaw12345 @huseyinacacak-janea