tests: refactored fs watch_dir tests for stability #480

whitlockjc · 2015-08-14T22:09:37Z

fs_event_watch_dir and fs_event_watch_dir_recursive could fail randomly
due to the way in which the tests were written. Originally timers were
used to create, remove and recreate the test files but this could lead
to a race condition if the timeout used to delete the test files ran
before all file creation fs events were handled. On top of that, the
file removal timer scheduled another timer to recreate the test files
and that timer's timeout could also lead to the same condition.

The refactoring removed timers for the removal/recreation of the test
files and instead the fs event callback was updated to have the
necessary logic to drive the test file removal. We no longer recreate
the test files since it appears to be unnecessary.

With the current PR, I can run 1000 tests for fs_event_watch_dir and
fs_event_watch_dir_recursive without a single failure, where prior to this
PR they would fail 14/100 on average.

Fixes #30

whitlockjc · 2015-08-14T22:09:54Z

/cc @saghul @indutny @misterdjules

whitlockjc · 2015-08-14T22:21:18Z

One thing I forgot to mention, or maybe it wasn't clear, is that the original test appeared to do four cycles of creating files and then removing the files. This current PR only does one cycle but updating it to do four should be trivial. It's your call as to whether I add the extra cycles back or not.

saghul · 2015-08-15T10:12:22Z

I'm afk until Monday. Will take a look then! Thanks!
On Aug 15, 2015 00:09, "Jeremy Whitlock" notifications@github.com wrote:

/cc @saghul https://github.com/saghul @indutny
https://github.com/indutny @misterdjules
https://github.com/misterdjules

—
Reply to this email directly or view it on GitHub
#480 (comment).

saghul · 2015-08-17T07:52:14Z

test/test-fs-event.c

+    fs_event_unlink_files(handle);
+  } else if (fs_event_cb_called == fs_event_created + fs_event_removed) {
+    /* Once we've processed all create and delete events, stop watching */
+    ASSERT(0 == uv_fs_event_stop(handle));


This is not really necessary, since calling uv_close will trigger a stop.

Makes sense. I didn't write the original test so I didn't want to change it but I will remove it if you think it's unnecessary.

saghul · 2015-08-17T07:56:12Z

CI: https://jenkins-iojs.nodesource.com/view/libuv/job/libuv+any-pr+multi/130/

saghul · 2015-08-17T08:06:15Z

Not sure if it was failing before, but SmartOS and FreeBSD are failing:

SmartOS:

`fs_event_watch_dir` failed: exit code 134
Output from process `fs_event_watch_dir`:
Assertion failed in test/test-fs-event.c on line 175: events == UV_RENAME

FreeBSD:

%  86|+ 216|-   7|T   0|S  12]: fs_event_watch_dir
`fs_event_watch_dir` failed: timeout
Output from process `fs_event_watch_dir`:
fs_event_cb_dir_multi_file called: 1
  created: 16
  removed: 0

whitlockjc · 2015-08-17T14:58:39Z

I will remove the unnecessary stuff and test on the other environments. I will update you.

saghul · 2015-08-17T14:59:22Z

Thanks! Ping me me if you need me to trigger the CI.

whitlockjc · 2015-08-17T16:00:30Z

I have updated the PR. I removed my debugging output (sorry), removed the unnecessary assertions @saghul mentioned and updated the assertion in the fs_event_cb_dir_multi_file callback so we do not have assertion failures on SmartOS. Fixing the assertion on SmartOS does not fix the test completely as it will still timeout like it does on FreeBSD but these tests were failing prior to my PR and appear to be a deeper libuv issue than the test race condition I've fixed. (A quick debugging session shows that on FreeBSD and SmartOS that the fs_event_cb_dir_multi_file callback is only invoked once instead instead of once per created/removed file.)

saghul · 2015-08-17T20:04:03Z

CI: https://jenkins-iojs.nodesource.com/view/libuv/job/libuv+any-pr+multi/132/

whitlockjc · 2015-08-18T01:06:13Z

I've found something. On FreeBSD (#128) and SmartOS, the tests were failing prior to this PR. But after looking at the CI environment, it seems something changed for fs_event_watch_dir because it use to pass on CentOS 7 but now it's failing there. That being said, I'll chase this down and update the PR.

whitlockjc · 2015-08-18T05:45:31Z

I've cleaned up the sources a little to remove some compiler warnings on non-OSX/Windows. Progress:

FreeBSD

FreeBSD was failing prior to this PR but I wanted to see if there was something I could do about it. What I've noticed is that the fs event callback fs_event_cb_dir_multi_file is called only once even though the test creates 16 files, which should result in 16 separate fs_event_cb_dir_multi_file invocations. dtruss -e -d -f ./test/run-tests fs_event_watch_dir shows only 3 kevent calls and that doesn't seem right but it seems to match up with what I'm seeing in my debugging. On a whim, I removed the EV_ONESHOT from https://github.com/libuv/libuv/blob/v1.x/src/unix/kqueue.c#L97 and the test passes but I know that is not the fix, and it could very well be a red herring. I'm working with @indutny on this but I don't know how much of his time I'll have.

CentOS

Building an environment as we speak. No idea why it would stop passing due to these changes but I'll find out soon enough.

SmartOS

SmartOS was failing prior to this PR as well and it has similar symptoms to FreeBSD. I will circle around to SmartOS when I make progress on one of the environments above.

whitlockjc · 2015-08-18T15:01:55Z

I've merged in v1.x changes and resolved conflicts. I'm testing on CentOS right now.

whitlockjc · 2015-08-18T15:15:02Z

I just tested locally on CentOS 7 and fs_event_watch_dir passes without issue. Testing FreeBSD again.

whitlockjc · 2015-08-18T15:33:35Z

I think we're back in the state we were prior to this PR. FreeBSD and SmartOS are failing as they were before and I think it might be a bigger issue not related to the previously flaky tests.

@saghul Can you kick off a new CI build to make sure CentOS 7 is passing again?

saghul · 2015-08-18T16:40:12Z

Thanks again for the effort you're putting on this. I'll trigger the CI as
soon as I'm back home.
On Aug 18, 2015 5:33 PM, "Jeremy Whitlock" notifications@github.com wrote:

I think we're back in the state we were prior to this PR. FreeBSD and
SmartOS are failing as they were before and I think it might be a bigger
issue.

@saghul https://github.com/saghul Can you kick off a new CI build to
make sure CentOS7 is passing again?

—
Reply to this email directly or view it on GitHub
#480 (comment).

whitlockjc · 2015-08-18T18:04:35Z

No problem. If you get a few minutes when you get home, ping me in IRC so we can chat.

indutny · 2015-08-18T20:37:51Z

I have investigated the FreeBSD side. I'm afraid this is just how it works: it coalesces events into one knote and returns them at kevent() call.

I think we could do following thing - make it create and unlink files in different event-loop ticks. Does it make sense?

whitlockjc · 2015-08-18T20:46:32Z

Yes. I will update the PR when I've got that approach implemented. Thanks for your help.

whitlockjc · 2015-08-18T21:46:06Z

The latest version of the PR has fs_event_watch_dir passing on FreeBSD, which should allow us to check that one off of #128 if/when this gets merged, and it is also passing on SmartOS now. (Both of these were failing prior to this PR.)

Once @indutny verified that the fs event callback fs_event_watch_dir was only being called once and was by design (expected), we realized the only way for this test to pass as written was to create one file per event loop tick and remove one file per event loop tick to make sure that the proper number of fs events were emitted for the created and removed files.

As it stands, this PR not only makes the fs_event_watch_dir and fs_event_watch_dir_recursive stable, it also fixes fs_event_watch_dir on FreeBSD, SmartOS and possibly others where it might had been failing. Thanks a lot @indutny and @misterdjules.

saghul · 2015-08-19T07:31:37Z

@whitlockjc sorry, yesterday ended up not working for me. CI: https://jenkins-iojs.nodesource.com/view/libuv/job/libuv+any-pr+multi/133/

saghul · 2015-08-19T09:43:05Z

@whitlockjc can you please rebase again? I pushed a fix for the broken build on Windows.

fs_event_watch_dir and fs_event_watch_dir_recursive could fail randomly due to the way in which the tests were written. Originally timers were used to create, remove and recreate the test files but this could lead to a race condition if the timeout used to delete the test files ran before all file creation fs events were handled. On top of that, the file removal timer scheduled another timer to recreate the test files and that timer's timeout could also lead to the same condition. The refactoring removed timers for the removal/recreation of the test files and instead the fs event callback was updated to have the necessary logic to drive the test file removal. We no longer recreate the test files since it appears to be unnecessary. Fixes libuv#30

whitlockjc · 2015-08-19T15:17:00Z

Done.

saghul · 2015-08-19T15:22:55Z

CI: https://jenkins-iojs.nodesource.com/view/libuv/job/libuv+any-pr+multi/135/

saghul · 2015-08-19T15:43:49Z

Great job @whitlockjc! LGTM 👍

whitlockjc · 2015-08-19T15:45:01Z

Thanks to all that helped: @indutny and @misterdjules Two excellent mentors.

fs_event_watch_dir and fs_event_watch_dir_recursive could fail randomly due to the way in which the tests were written. Originally timers were used to create, remove and recreate the test files but this could lead to a race condition if the timeout used to delete the test files ran before all file creation fs events were handled. On top of that, the file removal timer scheduled another timer to recreate the test files and that timer's timeout could also lead to the same condition. The refactoring removed timers for the removal/recreation of the test files and instead the fs event callback was updated to have the necessary logic to drive the test file removal. We no longer recreate the test files since it appears to be unnecessary. Fixes: #30 PR-URL: #480 Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>

saghul · 2015-08-19T20:17:14Z

Landed in ✨ 8326470 ✨ Much obliged, @whitlockjc!

whitlockjc · 2015-08-19T20:37:47Z

WOOHOO!

whitlockjc force-pushed the fix-fs_event_watch_dir branch from a51b3a8 to 7153f45 Compare August 14, 2015 22:17

whitlockjc changed the title ~~tests: Refactored fs watch_dir tests for stability~~ tests: refactored fs watch_dir tests for stability Aug 14, 2015

saghul reviewed Aug 17, 2015
View reviewed changes

whitlockjc force-pushed the fix-fs_event_watch_dir branch from 7153f45 to 6af56f8 Compare August 17, 2015 15:58

whitlockjc force-pushed the fix-fs_event_watch_dir branch from 6af56f8 to 2c80133 Compare August 18, 2015 05:37

whitlockjc force-pushed the fix-fs_event_watch_dir branch 2 times, most recently from 5da99b1 to edcb386 Compare August 18, 2015 15:00

whitlockjc force-pushed the fix-fs_event_watch_dir branch 2 times, most recently from 1506476 to a124253 Compare August 18, 2015 21:44

whitlockjc mentioned this pull request Aug 18, 2015

FreeBSD 10.1-release test failures #128

Closed

5 tasks

whitlockjc force-pushed the fix-fs_event_watch_dir branch from a124253 to f58ea51 Compare August 19, 2015 15:16

This was referenced Aug 19, 2015

6 Broken tests on SmartOS #232

Closed

fs: uv_fs_{open,read,close}_dir #416

Closed

saghul mentioned this pull request Aug 19, 2015

test: fs_event_watch_dir fails on OSX #30

Closed

saghul closed this Aug 19, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: refactored fs watch_dir tests for stability #480

tests: refactored fs watch_dir tests for stability #480

whitlockjc commented Aug 14, 2015

whitlockjc commented Aug 14, 2015

whitlockjc commented Aug 14, 2015

saghul commented Aug 15, 2015

saghul Aug 17, 2015

whitlockjc Aug 17, 2015

saghul commented Aug 17, 2015

saghul commented Aug 17, 2015

whitlockjc commented Aug 17, 2015

saghul commented Aug 17, 2015

whitlockjc commented Aug 17, 2015

saghul commented Aug 17, 2015

whitlockjc commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

saghul commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

indutny commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

saghul commented Aug 19, 2015

saghul commented Aug 19, 2015

whitlockjc commented Aug 19, 2015

saghul commented Aug 19, 2015

saghul commented Aug 19, 2015

whitlockjc commented Aug 19, 2015

saghul commented Aug 19, 2015

whitlockjc commented Aug 19, 2015

tests: refactored fs watch_dir tests for stability #480

tests: refactored fs watch_dir tests for stability #480

Conversation

whitlockjc commented Aug 14, 2015

whitlockjc commented Aug 14, 2015

whitlockjc commented Aug 14, 2015

saghul commented Aug 15, 2015

saghul Aug 17, 2015

Choose a reason for hiding this comment

whitlockjc Aug 17, 2015

Choose a reason for hiding this comment

saghul commented Aug 17, 2015

saghul commented Aug 17, 2015

whitlockjc commented Aug 17, 2015

saghul commented Aug 17, 2015

whitlockjc commented Aug 17, 2015

saghul commented Aug 17, 2015

whitlockjc commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

saghul commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

indutny commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

whitlockjc commented Aug 18, 2015

saghul commented Aug 19, 2015

saghul commented Aug 19, 2015

whitlockjc commented Aug 19, 2015

saghul commented Aug 19, 2015

saghul commented Aug 19, 2015

whitlockjc commented Aug 19, 2015

saghul commented Aug 19, 2015

whitlockjc commented Aug 19, 2015