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

test: fix flaky test-fs-watchfile on macOS #13252

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 27, 2017

On macOS, a watcher created with fs.watch() does not necessarily
start receiving events immediately. So it can miss a change by
fs.writefile() if it comes very soon after the watcher is created. Fix
test flakiness caused by this by using setInterval() to repeat the
write action.

Fixes: #13248

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test fs

On macOS, a watcher created with fs.watch() does not necessarily
start receiving events immediately. So it can miss a change by
fs.writefile() if it comes very soon after the watcher is created. Fix
test flakiness caused by this by using `setInterval()` to repeat the
write action.

Fixes: nodejs#13248
@Trott Trott added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels May 27, 2017
@Trott
Copy link
Member Author

Trott commented May 27, 2017

Stress tests are being run with -j 92 --repeat 100 test/parallel/test-fs-watchfile.js

Stress test this PR (should succeed): https://ci.nodejs.org/job/node-stress-single-test/nodes=osx1010/1238/console

Stress test master (should fail): https://ci.nodejs.org/job/node-stress-single-test/nodes=osx1010/1239/console (404 right now, but will appear when a host becomes available to run the test)

@Trott
Copy link
Member Author

Trott commented May 27, 2017

@Trott
Copy link
Member Author

Trott commented May 27, 2017

Stress tests came back as expected: master with failures, this PR all green.

fs.writeFile(`${dir}/foo.txt`, 'foo', common.mustCall(function(err) {
if (err) assert.fail(err);
}));
}, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] is 1 safe? does it not cause a busy-wait-ish situation?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe change to try {fs.writeFileSych ...?

Copy link
Member Author

Choose a reason for hiding this comment

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

is 1 safe?

It won't block the event loop or anything like that, if that's what you mean. The way intervals work, the next one only gets added to the queue after the first one finishes, and gets added to execute in the next tick of the event loop at the earliest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I figured it will not block the loop, but maybe stress the I/O. Although obviously the process shouldn't be doing anything else at this point.
I agree it's a least arbitrary value you could put here, so guess it could be read a sort of nextTickInterval idiom?

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@refack
Copy link
Contributor

refack commented May 29, 2017

@Trott you want to merge 35ae48c (maybe even just the common.refreshTmpDir();) into this?

Trott added a commit to Trott/io.js that referenced this pull request May 30, 2017
On macOS, a watcher created with fs.watch() does not necessarily
start receiving events immediately. So it can miss a change by
fs.writefile() if it comes very soon after the watcher is created. Fix
test flakiness caused by this by using `setInterval()` to repeat the
write action.

PR-URL: nodejs#13252
Fixes: nodejs#13248
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented May 30, 2017

Landed in 3b12a8d

@Trott Trott closed this May 30, 2017
jasnell pushed a commit that referenced this pull request Jun 5, 2017
On macOS, a watcher created with fs.watch() does not necessarily
start receiving events immediately. So it can miss a change by
fs.writefile() if it comes very soon after the watcher is created. Fix
test flakiness caused by this by using `setInterval()` to repeat the
write action.

PR-URL: #13252
Fixes: #13248
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request Jun 5, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

@Trott Trott deleted the watch-race branch January 13, 2022 22:45
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test-fs-watchfile failure on OSX - intermittent ?
8 participants