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 `fs-watch-recursive` flakiness #4629

Merged
merged 1 commit into from Sep 1, 2016

Conversation

Projects
None yet
6 participants
@santigimeno
Member

santigimeno commented Jan 11, 2016

The test is sometimes timing out because of a race condition between
the fs event generated on file creation and the event being registered
in the kqueue. To avoid this problem, create the file after 100 ms,
that is the value used in the libuv test:
https://github.com/libuv/libuv/blob/v1.x/test/test-fs-event.c#L411

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jan 11, 2016

Member

I really want their to be a better solution than "introduce a 100ms delay" but I don't know that there is a better solution. I suppose it theoretically may be possible to introduce a ready event that fires for fs.FSWatcher but then again, it might not be possible or only on some operating systems.

Speaking of which, what OS are you seeing this problem on? OS X?

Member

Trott commented Jan 11, 2016

I really want their to be a better solution than "introduce a 100ms delay" but I don't know that there is a better solution. I suppose it theoretically may be possible to introduce a ready event that fires for fs.FSWatcher but then again, it might not be possible or only on some operating systems.

Speaking of which, what OS are you seeing this problem on? OS X?

@santigimeno

This comment has been minimized.

Show comment
Hide comment
@santigimeno

santigimeno Jan 12, 2016

Member

Yes, I thought about the ready event too.
I'm seeing it on OS X.

Member

santigimeno commented Jan 12, 2016

Yes, I thought about the ready event too.
I'm seeing it on OS X.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 12, 2016

Member

Yeah, unfortunately right now I don't think there is a better solution :-(

Member

jasnell commented Jan 12, 2016

Yeah, unfortunately right now I don't think there is a better solution :-(

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jan 21, 2016

Member

Can we try this with setImmediate() instead of the setTimeout() and see if that fixes the issue? I'd prefer that if it will work.

Ref: #4776

Member

Trott commented Jan 21, 2016

Can we try this with setImmediate() instead of the setTimeout() and see if that fixes the issue? I'd prefer that if it will work.

Ref: #4776

@santigimeno

This comment has been minimized.

Show comment
Hide comment
@santigimeno

santigimeno Jan 21, 2016

Member

@Trott Unfortunately still times out :(

Member

santigimeno commented Jan 21, 2016

@Trott Unfortunately still times out :(

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 22, 2016

Member

@santigimeno @Trott ... any updates on this one?

Member

jasnell commented Mar 22, 2016

@santigimeno @Trott ... any updates on this one?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 22, 2016

Member

I'm OK with the timer if it's the only way we can come up with right now to make it less flaky. Maybe there's a feature request hiding in this problem for an event to be fired when the watcher is really watching.

Member

Trott commented Mar 22, 2016

I'm OK with the timer if it's the only way we can come up with right now to make it less flaky. Maybe there's a feature request hiding in this problem for an event to be fired when the watcher is really watching.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 22, 2016

Member

LGTM if @santigimeno is happy with it.

Member

Trott commented Mar 22, 2016

LGTM if @santigimeno is happy with it.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Mar 22, 2016

Member

(Note that it still needs a CI run, I think.)

Member

Trott commented Mar 22, 2016

(Note that it still needs a CI run, I think.)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@santigimeno

This comment has been minimized.

Show comment
Hide comment
@santigimeno

santigimeno Mar 23, 2016

Member

I'm OK until we find a better way.

Member

santigimeno commented Mar 23, 2016

I'm OK until we find a better way.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 30, 2016

Member

Ping... @santigimeno @Trott ... any updates on this?

Member

jasnell commented Apr 30, 2016

Ping... @santigimeno @Trott ... any updates on this?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 30, 2016

Member

It's been off my radar. Maybe @santigimeno knows (for example) if the test is still flaky and needs this fix.

Member

Trott commented Apr 30, 2016

It's been off my radar. Maybe @santigimeno knows (for example) if the test is still flaky and needs this fix.

@santigimeno

This comment has been minimized.

Show comment
Hide comment
@santigimeno

santigimeno Apr 30, 2016

Member

@Trott @jasnell I'm still seeing these failures in my OS X box. I'm also seeing similar failures in other fs-watch tests: test-fs-watch.js and test-fs-watch-encoding.js. I can land it if you're still OK with it, or we can wait until the flakiness happens in our CI. WDYT?

Member

santigimeno commented Apr 30, 2016

@Trott @jasnell I'm still seeing these failures in my OS X box. I'm also seeing similar failures in other fs-watch tests: test-fs-watch.js and test-fs-watch-encoding.js. I can land it if you're still OK with it, or we can wait until the flakiness happens in our CI. WDYT?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 30, 2016

Member

I've seen flakiness here from time to time building locally but it's
extremely rare. Curious, which OS x version are you on? I'd say let's hold
off still until it becomes an issue on CI
On Apr 30, 2016 6:24 AM, "Santiago Gimeno" notifications@github.com wrote:

@Trott https://github.com/Trott @jasnell https://github.com/jasnell
I'm still seeing these failures in my OS X box. I'm also seeing similar
failures in other fs-watch tests: test-fs-watch.js and
test-fs-watch-encoding.js. I can land it if you're still OK with it, or
we can wait until the flakiness happens in our CI. WDYT?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4629 (comment)

Member

jasnell commented Apr 30, 2016

I've seen flakiness here from time to time building locally but it's
extremely rare. Curious, which OS x version are you on? I'd say let's hold
off still until it becomes an issue on CI
On Apr 30, 2016 6:24 AM, "Santiago Gimeno" notifications@github.com wrote:

@Trott https://github.com/Trott @jasnell https://github.com/jasnell
I'm still seeing these failures in my OS X box. I'm also seeing similar
failures in other fs-watch tests: test-fs-watch.js and
test-fs-watch-encoding.js. I can land it if you're still OK with it, or
we can wait until the flakiness happens in our CI. WDYT?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4629 (comment)

@santigimeno

This comment has been minimized.

Show comment
Hide comment
@santigimeno

santigimeno Apr 30, 2016

Member

which OS x version are you on?

10.11.4

I'd say let's hold off still until it becomes an issue on CI

OK!

Member

santigimeno commented Apr 30, 2016

which OS x version are you on?

10.11.4

I'd say let's hold off still until it becomes an issue on CI

OK!

@santigimeno

This comment has been minimized.

Show comment
Hide comment
Member

santigimeno commented May 17, 2016

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jun 16, 2016

Member

OK, I think we should get this landed, but one other suggestion: Should we wrap the setTimeout() in a check for the OS so we only do that on OS X?

Member

Trott commented Jun 16, 2016

OK, I think we should get this landed, but one other suggestion: Should we wrap the setTimeout() in a check for the OS so we only do that on OS X?

@santigimeno

This comment has been minimized.

Show comment
Hide comment
@santigimeno

santigimeno Jul 1, 2016

Member

@Trott updated!

Member

santigimeno commented Jul 1, 2016

@Trott updated!

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott
Member

Trott commented Jul 1, 2016

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jul 1, 2016

Member

Nit: Maybe put in a brief comment explaining the need for the setTimeout on OS X. (kqueue or whatever can send events out of order, or whatever the correct explanation is. Maybe doesn't even need to get that detailed. Just link to this issue or something.)

Member

Trott commented Jul 1, 2016

Nit: Maybe put in a brief comment explaining the need for the setTimeout on OS X. (kqueue or whatever can send events out of order, or whatever the correct explanation is. Maybe doesn't even need to get that detailed. Just link to this issue or something.)

@Trott

This comment has been minimized.

Show comment
Hide comment
Member

Trott commented Jul 2, 2016

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Aug 31, 2016

Member

ping, can we get movement on this?

timeout failure on osx @ https://ci.nodejs.org/job/node-test-commit-linux/4919/nodes=ubuntu1204-clang341-64

Member

rvagg commented Aug 31, 2016

ping, can we get movement on this?

timeout failure on osx @ https://ci.nodejs.org/job/node-test-commit-linux/4919/nodes=ubuntu1204-clang341-64

@rvagg rvagg referenced this pull request Aug 31, 2016

Merged

deps: workaround clang-3.4 ICE #8343

2 of 2 tasks complete
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 1, 2016

Member

CI (since previous CI's are expired and 404'ing): https://ci.nodejs.org/job/node-test-pull-request/3916/

Member

Trott commented Sep 1, 2016

CI (since previous CI's are expired and 404'ing): https://ci.nodejs.org/job/node-test-pull-request/3916/

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 1, 2016

Member

AIX build failure is unrelated. Waiting on ubuntu1604_docker_alpine34-64 but everything looks good. Still LGTM.

Member

Trott commented Sep 1, 2016

AIX build failure is unrelated. Waiting on ubuntu1604_docker_alpine34-64 but everything looks good. Still LGTM.

test: fix `fs-watch-recursive` flakiness on OS X
The test is sometimes timing out because of a race condition between
the fs event generated on file creation and the event being registered
in the kqueue. To avoid this problem, create the file after 100 ms,
that is the value used in the `fs_event_watch_dir_recursive` libuv test.

PR-URL: #4629
Reviewed-By: Rich Trott <rtrott@gmail.com>

@santigimeno santigimeno merged commit a133b77 into nodejs:master Sep 1, 2016

@santigimeno

This comment has been minimized.

Show comment
Hide comment
@santigimeno

santigimeno Sep 1, 2016

Member

Landed in a133b77

Member

santigimeno commented Sep 1, 2016

Landed in a133b77

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

test: fix `fs-watch-recursive` flakiness on OS X
The test is sometimes timing out because of a race condition between
the fs event generated on file creation and the event being registered
in the kqueue. To avoid this problem, create the file after 100 ms,
that is the value used in the `fs_event_watch_dir_recursive` libuv test.

PR-URL: nodejs#4629
Reviewed-By: Rich Trott <rtrott@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

test: fix `fs-watch-recursive` flakiness on OS X
The test is sometimes timing out because of a race condition between
the fs event generated on file creation and the event being registered
in the kqueue. To avoid this problem, create the file after 100 ms,
that is the value used in the `fs_event_watch_dir_recursive` libuv test.

PR-URL: #4629
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 10, 2016

test: fix `fs-watch-recursive` flakiness on OS X
The test is sometimes timing out because of a race condition between
the fs event generated on file creation and the event being registered
in the kqueue. To avoid this problem, create the file after 100 ms,
that is the value used in the `fs_event_watch_dir_recursive` libuv test.

PR-URL: #4629
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 10, 2016

test: fix `fs-watch-recursive` flakiness on OS X
The test is sometimes timing out because of a race condition between
the fs event generated on file creation and the event being registered
in the kqueue. To avoid this problem, create the file after 100 ms,
that is the value used in the `fs_event_watch_dir_recursive` libuv test.

PR-URL: #4629
Reviewed-By: Rich Trott <rtrott@gmail.com>

rvagg added a commit that referenced this pull request Oct 18, 2016

test: fix `fs-watch-recursive` flakiness on OS X
The test is sometimes timing out because of a race condition between
the fs event generated on file creation and the event being registered
in the kqueue. To avoid this problem, create the file after 100 ms,
that is the value used in the `fs_event_watch_dir_recursive` libuv test.

PR-URL: #4629
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 26, 2016

test: fix `fs-watch-recursive` flakiness on OS X
The test is sometimes timing out because of a race condition between
the fs event generated on file creation and the event being registered
in the kqueue. To avoid this problem, create the file after 100 ms,
that is the value used in the `fs_event_watch_dir_recursive` libuv test.

PR-URL: #4629
Reviewed-By: Rich Trott <rtrott@gmail.com>

@MylesBorins MylesBorins referenced this pull request Oct 26, 2016

Closed

V4.6.2 proposal #9298

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment