From 83264701a7e7b7ad82eec6c601a208dfd71df74d Mon Sep 17 00:00:00 2001 From: Jeremy Whitlock Date: Fri, 14 Aug 2015 15:57:39 -0600 Subject: [PATCH] tests: refactored fs watch_dir tests for stability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/libuv/libuv/issues/30 PR-URL: https://github.com/libuv/libuv/pull/480 Reviewed-By: Saúl Ibarra Corretgé --- test/test-fs-event.c | 234 +++++++++++++++++++++++-------------------- 1 file changed, 123 insertions(+), 111 deletions(-) diff --git a/test/test-fs-event.c b/test/test-fs-event.c index 7d72d530..a0908ce5 100644 --- a/test/test-fs-event.c +++ b/test/test-fs-event.c @@ -37,14 +37,15 @@ static uv_fs_event_t fs_event; static const char file_prefix[] = "fsevent-"; +static const int fs_event_file_count = 16; #if defined(__APPLE__) || defined(_WIN32) static const char file_prefix_in_subdir[] = "subdir"; #endif static uv_timer_t timer; static int timer_cb_called; static int close_cb_called; -static const int fs_event_file_count = 128; static int fs_event_created; +static int fs_event_removed; static int fs_event_cb_called; #if defined(PATH_MAX) static char fs_event_filename[PATH_MAX]; @@ -53,9 +54,6 @@ static char fs_event_filename[1024]; #endif /* defined(PATH_MAX) */ static int timer_cb_touch_called; -static void fs_event_unlink_files(uv_timer_t* handle); -static void fs_event_unlink_files_in_subdir(uv_timer_t* handle); - static void create_dir(const char* name) { int r; uv_fs_t req; @@ -122,46 +120,6 @@ static void fs_event_cb_dir(uv_fs_event_t* handle, const char* filename, uv_close((uv_handle_t*)handle, close_cb); } -static void fs_event_cb_dir_multi_file(uv_fs_event_t* handle, - const char* filename, - int events, - int status) { - fs_event_cb_called++; - ASSERT(handle == &fs_event); - ASSERT(status == 0); - ASSERT(events == UV_RENAME); - ASSERT(filename == NULL || - strncmp(filename, file_prefix, sizeof(file_prefix) - 1) == 0); - - /* Stop watching dir when received events about all files: - * both create and close events */ - if (fs_event_cb_called == 2 * fs_event_file_count) { - ASSERT(0 == uv_fs_event_stop(handle)); - uv_close((uv_handle_t*) handle, close_cb); - } -} - -#if defined(__APPLE__) || defined(_WIN32) -static void fs_event_cb_dir_multi_file_in_subdir(uv_fs_event_t* handle, - const char* filename, - int events, - int status) { - fs_event_cb_called++; - ASSERT(handle == &fs_event); - ASSERT(status == 0); - ASSERT(events == UV_RENAME || events == UV_CHANGE); - ASSERT(filename == NULL || - strncmp(filename, file_prefix_in_subdir, sizeof(file_prefix_in_subdir) - 1) == 0); - - /* Stop watching dir when received events about all files: - * both create and close events */ - if (fs_event_cb_called == 2 * fs_event_file_count) { - ASSERT(0 == uv_fs_event_stop(handle)); - uv_close((uv_handle_t*) handle, close_cb); - } -} -#endif - static const char* fs_event_get_filename(int i) { snprintf(fs_event_filename, sizeof(fs_event_filename), @@ -171,6 +129,69 @@ static const char* fs_event_get_filename(int i) { return fs_event_filename; } +static void fs_event_create_files(uv_timer_t* handle) { + /* Make sure we're not attempting to create files we do not intend */ + ASSERT(fs_event_created < fs_event_file_count); + + /* Create the file */ + create_file(fs_event_get_filename(fs_event_created)); + + if (++fs_event_created < fs_event_file_count) { + /* Create another file on a different event loop tick. We do it this way + * to avoid fs events coalescing into one fs event. */ + ASSERT(0 == uv_timer_start(&timer, fs_event_create_files, 1, 0)); + } +} + +static void fs_event_unlink_files(uv_timer_t* handle) { + int r; + int i; + + /* NOTE: handle might be NULL if invoked not as timer callback */ + if (handle == NULL) { + /* Unlink all files */ + for (i = 0; i < 16; i++) { + r = remove(fs_event_get_filename(i)); + if (handle != NULL) + ASSERT(r == 0); + } + } else { + /* Make sure we're not attempting to remove files we do not intend */ + ASSERT(fs_event_removed < fs_event_file_count); + + /* Remove the file */ + ASSERT(0 == remove(fs_event_get_filename(fs_event_removed))); + + if (++fs_event_removed < fs_event_file_count) { + /* Remove another file on a different event loop tick. We do it this way + * to avoid fs events coalescing into one fs event. */ + ASSERT(0 == uv_timer_start(&timer, fs_event_unlink_files, 1, 0)); + } + } +} + +static void fs_event_cb_dir_multi_file(uv_fs_event_t* handle, + const char* filename, + int events, + int status) { + fs_event_cb_called++; + ASSERT(handle == &fs_event); + ASSERT(status == 0); + ASSERT(events == UV_CHANGE || UV_RENAME); + ASSERT(filename == NULL || + strncmp(filename, file_prefix, sizeof(file_prefix) - 1) == 0); + + if (fs_event_created + fs_event_removed == fs_event_file_count) { + /* Once we've processed all create events, delete all files */ + ASSERT(0 == uv_timer_start(&timer, fs_event_unlink_files, 1, 0)); + } else if (fs_event_cb_called == 2 * fs_event_file_count) { + /* Once we've processed all create and delete events, stop watching */ + uv_close((uv_handle_t*) &timer, close_cb); + uv_close((uv_handle_t*) handle, close_cb); + } +} + +#if defined(__APPLE__) || defined(_WIN32) static const char* fs_event_get_filename_in_subdir(int i) { snprintf(fs_event_filename, sizeof(fs_event_filename), @@ -180,75 +201,68 @@ static const char* fs_event_get_filename_in_subdir(int i) { return fs_event_filename; } -static void fs_event_create_files(uv_timer_t* handle) { - int i; - - /* Already created all files */ - if (fs_event_created == fs_event_file_count) { - uv_close((uv_handle_t*) &timer, close_cb); - return; - } - - /* Create all files */ - for (i = 0; i < 16; i++, fs_event_created++) - create_file(fs_event_get_filename(i)); - - /* And unlink them */ - ASSERT(0 == uv_timer_start(&timer, fs_event_unlink_files, 50, 0)); -} - static void fs_event_create_files_in_subdir(uv_timer_t* handle) { + /* Make sure we're not attempting to create files we do not intend */ + ASSERT(fs_event_created < fs_event_file_count); + + /* Create the file */ + create_file(fs_event_get_filename_in_subdir(fs_event_created)); + + if (++fs_event_created < fs_event_file_count) { + /* Create another file on a different event loop tick. We do it this way + * to avoid fs events coalescing into one fs event. */ + ASSERT(0 == uv_timer_start(&timer, fs_event_create_files_in_subdir, 1, 0)); + } +} + +static void fs_event_unlink_files_in_subdir(uv_timer_t* handle) { + int r; int i; - /* Already created all files */ - if (fs_event_created == fs_event_file_count) { + /* NOTE: handle might be NULL if invoked not as timer callback */ + if (handle == NULL) { + /* Unlink all files */ + for (i = 0; i < 16; i++) { + r = remove(fs_event_get_filename_in_subdir(i)); + if (handle != NULL) + ASSERT(r == 0); + } + } else { + /* Make sure we're not attempting to remove files we do not intend */ + ASSERT(fs_event_removed < fs_event_file_count); + + /* Remove the file */ + ASSERT(0 == remove(fs_event_get_filename_in_subdir(fs_event_removed))); + + if (++fs_event_removed < fs_event_file_count) { + /* Remove another file on a different event loop tick. We do it this way + * to avoid fs events coalescing into one fs event. */ + ASSERT(0 == uv_timer_start(&timer, fs_event_unlink_files_in_subdir, 1, 0)); + } + } +} + +static void fs_event_cb_dir_multi_file_in_subdir(uv_fs_event_t* handle, + const char* filename, + int events, + int status) { + fs_event_cb_called++; + ASSERT(handle == &fs_event); + ASSERT(status == 0); + ASSERT(events == UV_CHANGE || UV_RENAME); + ASSERT(filename == NULL || + strncmp(filename, file_prefix_in_subdir, sizeof(file_prefix_in_subdir) - 1) == 0); + + if (fs_event_created + fs_event_removed == fs_event_file_count) { + /* Once we've processed all create events, delete all files */ + ASSERT(0 == uv_timer_start(&timer, fs_event_unlink_files_in_subdir, 1, 0)); + } else if (fs_event_cb_called == 2 * fs_event_file_count) { + /* Once we've processed all create and delete events, stop watching */ uv_close((uv_handle_t*) &timer, close_cb); - return; + uv_close((uv_handle_t*) handle, close_cb); } - - /* Create all files */ - for (i = 0; i < 16; i++, fs_event_created++) - create_file(fs_event_get_filename_in_subdir(i)); - - /* And unlink them */ - ASSERT(0 == uv_timer_start(&timer, fs_event_unlink_files_in_subdir, 50, 0)); -} - -void fs_event_unlink_files_in_subdir(uv_timer_t* handle) { - int r; - int i; - - /* NOTE: handle might be NULL if invoked not as timer callback */ - - /* Unlink all files */ - for (i = 0; i < 16; i++) { - r = remove(fs_event_get_filename_in_subdir(i)); - if (handle != NULL) - ASSERT(r == 0); - } - - /* And create them again */ - if (handle != NULL) - ASSERT(0 == uv_timer_start(&timer, fs_event_create_files_in_subdir, 50, 0)); -} - -void fs_event_unlink_files(uv_timer_t* handle) { - int r; - int i; - - /* NOTE: handle might be NULL if invoked not as timer callback */ - - /* Unlink all files */ - for (i = 0; i < 16; i++) { - r = remove(fs_event_get_filename(i)); - if (handle != NULL) - ASSERT(r == 0); - } - - /* And create them again */ - if (handle != NULL) - ASSERT(0 == uv_timer_start(&timer, fs_event_create_files, 50, 0)); } +#endif static void fs_event_cb_file(uv_fs_event_t* handle, const char* filename, int events, int status) { @@ -336,8 +350,7 @@ TEST_IMPL(fs_event_watch_dir) { uv_run(loop, UV_RUN_DEFAULT); - ASSERT(fs_event_cb_called == 2 * fs_event_file_count); - ASSERT(fs_event_created == fs_event_file_count); + ASSERT(fs_event_cb_called == fs_event_created + fs_event_removed); ASSERT(close_cb_called == 2); /* Cleanup */ @@ -376,8 +389,7 @@ TEST_IMPL(fs_event_watch_dir_recursive) { uv_run(loop, UV_RUN_DEFAULT); - ASSERT(fs_event_cb_called == 2 * fs_event_file_count); - ASSERT(fs_event_created == fs_event_file_count); + ASSERT(fs_event_cb_called == fs_event_created + fs_event_removed); ASSERT(close_cb_called == 2); /* Cleanup */