Skip to content

Conversation

@Trott
Copy link
Member

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
@TrottTrott 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
MemberAuthor

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
MemberAuthor

@Trott
Copy link
MemberAuthor

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
MemberAuthor

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

@mhdawsonmhdawson 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

@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#13252Fixes: 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
MemberAuthor

Landed in 3b12a8d

@TrottTrott 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: #13252Fixes: #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]>
@jasnelljasnell mentioned this pull request Jun 5, 2017
@gibfahngibfahn 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

@TrottTrott deleted the watch-race branch January 13, 2022 22:45
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fsIssues and PRs related to the fs subsystem / file system.testIssues 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

@Trott@refack@MylesBorins@jasnell@benjamingr@lpinca@cjihrig@mhdawson