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

Issue with fs-watch tests on macOS with incoming [email protected] release #54450

Closed
santigimeno opened this issue Aug 19, 2024 · 3 comments
Closed
Labels
fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. macos Issues and PRs related to the macOS platform / OSX.

Comments

@santigimeno
Copy link
Member

We're in the process of releasing [email protected]. While running the libuv-in-node CI bot, many of the fs-watch tests in macOS have become very flaky. It looks like the root of the problem comes from how fsevents on macOS are handled (explanation here and here), though these issues have become more apparent after a change in libuv which fixed some other issues such as: #52055. That, until now, this problem wasn't appearing was probably by chance as there was no test that actually exercised that specific flow. Here's an example of code that demonstrates the issue in current Node.js main:

const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path');
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'foo-'));
console.log('dir', dir);
const filePath = path.join(dir, 'test');
const content1 = Date.now() + 'test'.toLowerCase().repeat(1e4);
fs.writeFileSync(filePath, content1);
// In macOs the events from before calling fs.watch() may be leaked into
// the 'change' event. Uncomment the timeout to avoid it.
// setTimeout(() => {
const watcher = fs.watch(dir);
watcher.on('change', (type, filename) => {
  console.log(type, filename);
});
// }, 100);

This issue might probably be fixed once libuv/libuv#3866 is fixed, but that's not going to happen soon and definitely not for this release.
So, in order to land the incoming [email protected] my question is:

  1. Would it be acceptable to rewrite those fs.watch tests so they take into account the possible leaking of events generated before the call to fs.watch()?
  2. Or you'd prefer reverting the fix for macOS: fs.watch does not report delete of watched folder #52055 which would make the test suite pass though the underlying issue would still be there.

I'm asking this before I want to be sure Option 1 would be acceptable before rewriting all those tests, which I'm happy to pursue.

@santigimeno santigimeno changed the title Issue with fs-watch events with incoming [email protected] release Issue with fs-watch tests with incoming [email protected] release Aug 19, 2024
@santigimeno santigimeno changed the title Issue with fs-watch tests with incoming [email protected] release Issue with fs-watch tests on macOS with incoming [email protected] release Aug 19, 2024
@santigimeno santigimeno added fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. labels Aug 19, 2024
@santigimeno
Copy link
Member Author

/cc @MoLow

@RedYetiDev RedYetiDev added the macos Issues and PRs related to the macOS platform / OSX. label Aug 19, 2024
@lpinca
Copy link
Member

lpinca commented Aug 20, 2024

Would it be acceptable to rewrite those fs.watch tests so they take into account the possible leaking of events generated before the call to fs.watch()?

I think it is acceptable.

santigimeno added a commit to santigimeno/node that referenced this issue Aug 22, 2024
In `macOS`, fsevents generated immediately before start watching may
leak into the event callback. See: nodejs#54450
for an explanation. This might be fixed at some point in `libuv`
(See: libuv/libuv#3866).
This commit comes in anticipation of the soon-to-be-released
`[email protected]`.
santigimeno added a commit to santigimeno/node that referenced this issue Aug 22, 2024
In `macOS`, fsevents generated immediately before start watching may
leak into the event callback. See: nodejs#54450
for an explanation. This might be fixed at some point in `libuv` though
it may take some time (see: libuv/libuv#3866).
This commit comes in anticipation of the soon-to-be-released
`[email protected]` which was making these tests very flaky.
nodejs-github-bot pushed a commit that referenced this issue Sep 6, 2024
In `macOS`, fsevents generated immediately before start watching may
leak into the event callback. See: #54450
for an explanation. This might be fixed at some point in `libuv` though
it may take some time (see: libuv/libuv#3866).
This commit comes in anticipation of the soon-to-be-released
`[email protected]` which was making these tests very flaky.

PR-URL: #54498
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
aduh95 pushed a commit that referenced this issue Sep 12, 2024
In `macOS`, fsevents generated immediately before start watching may
leak into the event callback. See: #54450
for an explanation. This might be fixed at some point in `libuv` though
it may take some time (see: libuv/libuv#3866).
This commit comes in anticipation of the soon-to-be-released
`[email protected]` which was making these tests very flaky.

PR-URL: #54498
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
targos pushed a commit that referenced this issue Sep 22, 2024
In `macOS`, fsevents generated immediately before start watching may
leak into the event callback. See: #54450
for an explanation. This might be fixed at some point in `libuv` though
it may take some time (see: libuv/libuv#3866).
This commit comes in anticipation of the soon-to-be-released
`[email protected]` which was making these tests very flaky.

PR-URL: #54498
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
targos pushed a commit that referenced this issue Sep 26, 2024
In `macOS`, fsevents generated immediately before start watching may
leak into the event callback. See: #54450
for an explanation. This might be fixed at some point in `libuv` though
it may take some time (see: libuv/libuv#3866).
This commit comes in anticipation of the soon-to-be-released
`[email protected]` which was making these tests very flaky.

PR-URL: #54498
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
targos pushed a commit that referenced this issue Oct 2, 2024
In `macOS`, fsevents generated immediately before start watching may
leak into the event callback. See: #54450
for an explanation. This might be fixed at some point in `libuv` though
it may take some time (see: libuv/libuv#3866).
This commit comes in anticipation of the soon-to-be-released
`[email protected]` which was making these tests very flaky.

PR-URL: #54498
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
In `macOS`, fsevents generated immediately before start watching may
leak into the event callback. See: nodejs#54450
for an explanation. This might be fixed at some point in `libuv` though
it may take some time (see: libuv/libuv#3866).
This commit comes in anticipation of the soon-to-be-released
`[email protected]` which was making these tests very flaky.

PR-URL: nodejs#54498
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@santigimeno
Copy link
Member Author

This is done.

tpoisseau pushed a commit to tpoisseau/node that referenced this issue Nov 21, 2024
In `macOS`, fsevents generated immediately before start watching may
leak into the event callback. See: nodejs#54450
for an explanation. This might be fixed at some point in `libuv` though
it may take some time (see: libuv/libuv#3866).
This commit comes in anticipation of the soon-to-be-released
`[email protected]` which was making these tests very flaky.

PR-URL: nodejs#54498
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. macos Issues and PRs related to the macOS platform / OSX.
Projects
None yet
Development

No branches or pull requests

3 participants