Browse Source

fix: fs.watch() behavior change in node >= 10.16.0 (#20433)

This reverts the patch from https://github.com/electron/node/pull/100
which never got merged due to reasons outlined in https://github.com/libuv/libuv/pull/2313

* Adds new patches that backports https://github.com/libuv/libuv/pull/2459
  and https://github.com/libuv/libuv/pull/2460

Based on https://github.com/nodejs/node/issues/29460
Robo 5 years ago
parent
commit
486a8ea880

+ 2 - 1
patches/common/node/.patches

@@ -36,10 +36,11 @@ feat_add_original-fs_module.patch
 build_allow_embedders_to_override_the_node_module_version_define.patch
 refactor_allow_embedder_overriding_of_internal_fs_calls.patch
 chore_add_ability_to_prevent_warn_non_context-aware_native_modules.patch
-fsevents_fix_file_event_reporting.patch
 src_only_run_preloadmodules_if_the_preload_array_is_not_empty.patch
 src_read_break_node_first_line_from_the_inspect_options.patch
 chore_allow_the_node_entrypoint_to_be_a_builtin_module.patch
 inherit_electron_crashpad_pipe_name_in_child_process.patch
 http2_fix_tracking_received_data_for_maxsessionmemory.patch
 fix_set_uptime_offset_in_correct_init_method.patch
+fsevents-stop-using-fsevents-to-watch-files.patch
+fsevents-regression-in-watching.patch

+ 158 - 0
patches/common/node/fsevents-regression-in-watching.patch

@@ -0,0 +1,158 @@
+From 50a8c4ff489bad8a93159e0cb46e17c6cde2d5a6 Mon Sep 17 00:00:00 2001
+From: Jameson Nash <[email protected]>
+Date: Sat, 7 Sep 2019 20:45:39 -0400
+Subject: [PATCH] fsevents: regression in watching /
+
+This case got lost by accident in
+https://github.com/libuv/libuv/pull/2082,
+preventing the realpath `/` from ever matching.
+
+Fixes: https://github.com/nodejs/node/issues/28917
+PR-URL: https://github.com/libuv/libuv/pull/2460
+Reviewed-By: Ben Noordhuis <[email protected]>
+Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
+
+diff --git a/deps/uv/src/unix/fsevents.c b/deps/uv/src/unix/fsevents.c
+index c430562b..08339f1f 100644
+--- a/deps/uv/src/unix/fsevents.c
++++ b/deps/uv/src/unix/fsevents.c
+@@ -262,10 +262,12 @@ static void uv__fsevents_event_cb(ConstFSEventStreamRef streamRef,
+       if (len < handle->realpath_len)
+         continue;
+ 
++      /* Make sure that realpath actually named a directory,
++       * (unless watching root, which alone keeps a trailing slash on the realpath)
++       * or that we matched the whole string */
+       if (handle->realpath_len != len &&
++          handle->realpath_len > 1 &&
+           path[handle->realpath_len] != '/')
+-        /* Make sure that realpath actually named a directory,
+-         * or that we matched the whole string */
+         continue;
+ 
+       if (memcmp(path, handle->realpath, handle->realpath_len) != 0)
+diff --git a/deps/uv/test/test-fs-event.c b/deps/uv/test/test-fs-event.c
+index 4b8bb1ef..7725c3af 100644
+--- a/deps/uv/test/test-fs-event.c
++++ b/deps/uv/test/test-fs-event.c
+@@ -47,6 +47,7 @@ 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";
++static int fs_multievent_cb_called;
+ #endif
+ static uv_timer_t timer;
+ static int timer_cb_called;
+@@ -280,7 +281,7 @@ static void fs_event_cb_dir_multi_file_in_subdir(uv_fs_event_t* handle,
+   if (filename && strcmp(filename, file_prefix_in_subdir) == 0)
+     return;
+ #endif
+-  fs_event_cb_called++;
++  fs_multievent_cb_called++;
+   ASSERT(handle == &fs_event);
+   ASSERT(status == 0);
+   ASSERT(events == UV_CHANGE || events == UV_RENAME);
+@@ -298,7 +299,7 @@ static void fs_event_cb_dir_multi_file_in_subdir(uv_fs_event_t* handle,
+   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) {
++  } else if (fs_multievent_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);
+@@ -393,6 +394,21 @@ static void timer_cb_watch_twice(uv_timer_t* handle) {
+   uv_close((uv_handle_t*) handle, NULL);
+ }
+ 
++static void fs_event_cb_close(uv_fs_event_t* handle,
++                              const char* filename,
++                              int events,
++                              int status) {
++  ASSERT(status == 0);
++
++  ASSERT(fs_event_cb_called < 3);
++  ++fs_event_cb_called;
++
++  if (fs_event_cb_called == 3) {
++    uv_close((uv_handle_t*) handle, close_cb);
++  }
++}
++
++
+ TEST_IMPL(fs_event_watch_dir) {
+ #if defined(NO_FS_EVENTS)
+   RETURN_SKIP(NO_FS_EVENTS);
+@@ -434,10 +450,12 @@ TEST_IMPL(fs_event_watch_dir) {
+   return 0;
+ }
+ 
++
+ TEST_IMPL(fs_event_watch_dir_recursive) {
+ #if defined(__APPLE__) || defined(_WIN32)
+   uv_loop_t* loop;
+   int r;
++  uv_fs_event_t fs_event_root;
+ 
+   /* Setup */
+   loop = uv_default_loop();
+@@ -451,17 +469,37 @@ TEST_IMPL(fs_event_watch_dir_recursive) {
+ 
+   r = uv_fs_event_init(loop, &fs_event);
+   ASSERT(r == 0);
+-  r = uv_fs_event_start(&fs_event, fs_event_cb_dir_multi_file_in_subdir, "watch_dir", UV_FS_EVENT_RECURSIVE);
++  r = uv_fs_event_start(&fs_event,
++                        fs_event_cb_dir_multi_file_in_subdir,
++                        "watch_dir",
++                        UV_FS_EVENT_RECURSIVE);
+   ASSERT(r == 0);
+   r = uv_timer_init(loop, &timer);
+   ASSERT(r == 0);
+   r = uv_timer_start(&timer, fs_event_create_files_in_subdir, 100, 0);
+   ASSERT(r == 0);
+ 
++#ifndef _WIN32
++  /* Also try to watch the root directory.
++   * This will be noisier, so we're just checking for any couple events to happen. */
++  r = uv_fs_event_init(loop, &fs_event_root);
++  ASSERT(r == 0);
++  r = uv_fs_event_start(&fs_event_root,
++                        fs_event_cb_close,
++                        "/",
++                        UV_FS_EVENT_RECURSIVE);
++  ASSERT(r == 0);
++#else
++  fs_event_cb_called += 3;
++  close_cb_called += 1;
++  (void)fs_event_root;
++#endif
++
+   uv_run(loop, UV_RUN_DEFAULT);
+ 
+-  ASSERT(fs_event_cb_called == fs_event_created + fs_event_removed);
+-  ASSERT(close_cb_called == 2);
++  ASSERT(fs_multievent_cb_called == fs_event_created + fs_event_removed);
++  ASSERT(fs_event_cb_called == 3);
++  ASSERT(close_cb_called == 3);
+ 
+   /* Cleanup */
+   fs_event_unlink_files_in_subdir(NULL);
+@@ -870,18 +908,6 @@ TEST_IMPL(fs_event_close_with_pending_event) {
+   return 0;
+ }
+ 
+-static void fs_event_cb_close(uv_fs_event_t* handle, const char* filename,
+-    int events, int status) {
+-  ASSERT(status == 0);
+-
+-  ASSERT(fs_event_cb_called < 3);
+-  ++fs_event_cb_called;
+-
+-  if (fs_event_cb_called == 3) {
+-    uv_close((uv_handle_t*) handle, close_cb);
+-  }
+-}
+-
+ TEST_IMPL(fs_event_close_in_callback) {
+ #if defined(NO_FS_EVENTS)
+   RETURN_SKIP(NO_FS_EVENTS);

+ 124 - 0
patches/common/node/fsevents-stop-using-fsevents-to-watch-files.patch

@@ -0,0 +1,124 @@
+From 9f23fab378a8a367418f1454c147cd7f96b15b1a Mon Sep 17 00:00:00 2001
+From: Jameson Nash <[email protected]>
+Date: Sat, 7 Sep 2019 14:55:40 -0400
+Subject: [PATCH] fsevents: stop using fsevents to watch files
+
+Goes back to just using it to watch folders,
+but keeps the other logic changes around.
+
+Refs: https://github.com/libuv/libuv/pull/387
+Refs: https://github.com/libuv/libuv/pull/2082
+Refs: https://github.com/libuv/libuv/pull/1572
+Refs: https://github.com/nodejs/node/issues/29460
+Fixes: https://github.com/libuv/libuv/issues/2488
+Closes: https://github.com/libuv/libuv/pull/2452
+PR-URL: https://github.com/libuv/libuv/pull/2459
+Reviewed-By: Ben Noordhuis <[email protected]>
+Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
+
+diff --git a/deps/uv/src/unix/kqueue.c b/deps/uv/src/unix/kqueue.c
+index 09200516..f5b5782c 100644
+--- a/deps/uv/src/unix/kqueue.c
++++ b/deps/uv/src/unix/kqueue.c
+@@ -454,25 +454,44 @@ int uv_fs_event_start(uv_fs_event_t* handle,
+                       const char* path,
+                       unsigned int flags) {
+   int fd;
++#if defined(__APPLE__) && MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
++  struct stat statbuf;
++#endif
+ 
+   if (uv__is_active(handle))
+     return UV_EINVAL;
+ 
+-#if defined(__APPLE__)
++  handle->cb = cb;
++  handle->path = uv__strdup(path);
++  if (handle->path == NULL)
++    return UV_ENOMEM;
++
++  /* TODO open asynchronously - but how do we report back errors? */
++  fd = open(handle->path, O_RDONLY);
++  if (fd == -1) {
++    uv__free(handle->path);
++    handle->path = NULL;
++    return UV__ERR(errno);
++  }
++
++#if defined(__APPLE__) && MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
+   /* Nullify field to perform checks later */
+   handle->cf_cb = NULL;
+   handle->realpath = NULL;
+   handle->realpath_len = 0;
+   handle->cf_flags = flags;
+ 
++  if (fstat(fd, &statbuf))
++    goto fallback;
++  /* FSEvents works only with directories */
++  if (!(statbuf.st_mode & S_IFDIR))
++    goto fallback;
++
+   if (!uv__has_forked_with_cfrunloop) {
+     int r;
+-    /* The fallback fd is not used */
++    /* The fallback fd is no longer needed */
++    uv__close_nocheckstdio(fd);
+     handle->event_watcher.fd = -1;
+-    handle->path = uv__strdup(path);
+-    if (handle->path == NULL)
+-      return UV_ENOMEM;
+-    handle->cb = cb;
+     r = uv__fsevents_init(handle);
+     if (r == 0) {
+       uv__handle_start(handle);
+@@ -482,20 +501,9 @@ int uv_fs_event_start(uv_fs_event_t* handle,
+     }
+     return r;
+   }
+-#endif /* defined(__APPLE__) */
+-
+-  /* TODO open asynchronously - but how do we report back errors? */
+-  fd = open(path, O_RDONLY);
+-  if (fd == -1)
+-    return UV__ERR(errno);
++fallback:
++#endif /* #if defined(__APPLE__) && MAC_OS_X_VERSION_MAX_ALLOWED >= 1070 */
+ 
+-  handle->path = uv__strdup(path);
+-  if (handle->path == NULL) {
+-    uv__close_nocheckstdio(fd);
+-    return UV_ENOMEM;
+-  }
+-
+-  handle->cb = cb;
+   uv__handle_start(handle);
+   uv__io_init(&handle->event_watcher, uv__fs_event, fd);
+   uv__io_start(handle->loop, &handle->event_watcher, POLLIN);
+@@ -513,8 +521,8 @@ int uv_fs_event_stop(uv_fs_event_t* handle) {
+ 
+   uv__handle_stop(handle);
+ 
+-#if defined(__APPLE__)
+-  if (!uv__has_forked_with_cfrunloop)
++#if defined(__APPLE__) && MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
++  if (!uv__has_forked_with_cfrunloop && handle->cf_cb != NULL)
+     r = uv__fsevents_close(handle);
+ #endif
+ 
+diff --git a/deps/uv/test/test-fs-event.c b/deps/uv/test/test-fs-event.c
+index ea34bd63..4b8bb1ef 100644
+--- a/deps/uv/test/test-fs-event.c
++++ b/deps/uv/test/test-fs-event.c
+@@ -656,6 +656,12 @@ TEST_IMPL(fs_event_watch_file_current_dir) {
+   /* Setup */
+   remove("watch_file");
+   create_file("watch_file");
++#if defined(__APPLE__) && !defined(MAC_OS_X_VERSION_10_12)
++  /* Empirically, kevent seems to (sometimes) report the preceeding
++   * create_file events prior to macOS 10.11.6 in the subsequent fs_event_start
++   * So let the system settle before running the test. */
++  uv_sleep(1100);
++#endif
+ 
+   r = uv_fs_event_init(loop, &fs_event);
+   ASSERT(r == 0);

+ 0 - 213
patches/common/node/fsevents_fix_file_event_reporting.patch

@@ -1,213 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Ben Noordhuis <[email protected]>
-Date: Mon, 27 May 2019 14:31:53 +0200
-Subject: fsevents: fix file event reporting
-
-Commit 2d2af38 ("fsevents: really watch files with fsevents on macos
-10.7+") from last November introduced a regression where events that
-were previously reported as UV_RENAME were now reported as UV_CHANGE.
-This commit rectifies that.
-
-Fixes: https://github.com/nodejs/node/issues/27869
-
-diff --git a/deps/uv/src/unix/fsevents.c b/deps/uv/src/unix/fsevents.c
-index c430562b37298a0065f6d39ecbb7e2857c863ca1..fd94df5e68a44beda683df99e6af7cc49350cb55 100644
---- a/deps/uv/src/unix/fsevents.c
-+++ b/deps/uv/src/unix/fsevents.c
-@@ -48,19 +48,9 @@ void uv__fsevents_loop_delete(uv_loop_t* loop) {
- #include <CoreFoundation/CFRunLoop.h>
- #include <CoreServices/CoreServices.h>
- 
--/* These are macros to avoid "initializer element is not constant" errors
-+/* Macro to avoid "initializer element is not constant" errors
-  * with old versions of gcc.
-  */
--#define kFSEventsModified (kFSEventStreamEventFlagItemFinderInfoMod |         \
--                           kFSEventStreamEventFlagItemModified |              \
--                           kFSEventStreamEventFlagItemInodeMetaMod |          \
--                           kFSEventStreamEventFlagItemChangeOwner |           \
--                           kFSEventStreamEventFlagItemXattrMod)
--
--#define kFSEventsRenamed  (kFSEventStreamEventFlagItemCreated |               \
--                           kFSEventStreamEventFlagItemRemoved |               \
--                           kFSEventStreamEventFlagItemRenamed)
--
- #define kFSEventsSystem   (kFSEventStreamEventFlagUserDropped |               \
-                            kFSEventStreamEventFlagKernelDropped |             \
-                            kFSEventStreamEventFlagEventIdsWrapped |           \
-@@ -288,8 +278,6 @@ static void uv__fsevents_event_cb(ConstFSEventStreamRef streamRef,
-             path--;
-             len++;
-           }
--          /* Created and Removed seem to be always set, but don't make sense */
--          flags &= ~kFSEventsRenamed;
-         } else {
-           /* Skip forward slash */
-           path++;
-@@ -310,12 +298,12 @@ static void uv__fsevents_event_cb(ConstFSEventStreamRef streamRef,
- 
-       memset(event, 0, sizeof(*event));
-       memcpy(event->path, path, len + 1);
--      event->events = UV_RENAME;
-+      event->events = UV_CHANGE;
- 
--      if (0 == (flags & kFSEventsRenamed)) {
--        if (0 != (flags & kFSEventsModified) ||
--            0 == (flags & kFSEventStreamEventFlagItemIsDir))
--          event->events = UV_CHANGE;
-+      if ((flags & kFSEventStreamEventFlagItemIsDir) ||
-+          (flags & kFSEventStreamEventFlagItemRemoved) ||
-+          (flags & kFSEventStreamEventFlagItemRenamed)) {
-+        event->events = UV_RENAME;
-       }
- 
-       QUEUE_INSERT_TAIL(&head, &event->member);
-diff --git a/deps/uv/test/test-fs-event.c b/deps/uv/test/test-fs-event.c
-index ea34bd63a70625c3e2c60d5a1bbb087c5f0bbb2e..38d722a27ef1a78717726272d54578c734280242 100644
---- a/deps/uv/test/test-fs-event.c
-+++ b/deps/uv/test/test-fs-event.c
-@@ -62,6 +62,15 @@ static char fs_event_filename[1024];
- static int timer_cb_touch_called;
- static int timer_cb_exact_called;
- 
-+static void expect_filename(const char* path, const char* expected) {
-+#if defined(__APPLE__) || defined(_WIN32) || defined(__linux__)
-+  ASSERT(0 == strcmp(path, expected));
-+#else
-+  if (path != NULL)
-+    ASSERT(0 == strcmp(path, expected));
-+#endif
-+}
-+
- static void fs_event_fail(uv_fs_event_t* handle,
-                           const char* filename,
-                           int events,
-@@ -130,11 +139,7 @@ static void fs_event_cb_dir(uv_fs_event_t* handle, const char* filename,
-   ASSERT(handle == &fs_event);
-   ASSERT(status == 0);
-   ASSERT(events == UV_CHANGE);
--  #if defined(__APPLE__) || defined(_WIN32) || defined(__linux__)
--  ASSERT(strcmp(filename, "file1") == 0);
--  #else
--  ASSERT(filename == NULL || strcmp(filename, "file1") == 0);
--  #endif
-+  expect_filename(filename, "file1");
-   ASSERT(0 == uv_fs_event_stop(handle));
-   uv_close((uv_handle_t*)handle, close_cb);
- }
-@@ -312,11 +317,7 @@ static void fs_event_cb_file(uv_fs_event_t* handle, const char* filename,
-   ASSERT(handle == &fs_event);
-   ASSERT(status == 0);
-   ASSERT(events == UV_CHANGE);
--  #if defined(__APPLE__) || defined(_WIN32) || defined(__linux__)
--  ASSERT(strcmp(filename, "file2") == 0);
--  #else
--  ASSERT(filename == NULL || strcmp(filename, "file2") == 0);
--  #endif
-+  expect_filename(filename, "file2");
-   ASSERT(0 == uv_fs_event_stop(handle));
-   uv_close((uv_handle_t*)handle, close_cb);
- }
-@@ -339,11 +340,7 @@ static void fs_event_cb_file_current_dir(uv_fs_event_t* handle,
-   ASSERT(handle == &fs_event);
-   ASSERT(status == 0);
-   ASSERT(events == UV_CHANGE);
--  #if defined(__APPLE__) || defined(_WIN32) || defined(__linux__)
--  ASSERT(strcmp(filename, "watch_file") == 0);
--  #else
--  ASSERT(filename == NULL || strcmp(filename, "watch_file") == 0);
--  #endif
-+  expect_filename(filename, "watch_file");
- 
-   /* Regression test for SunOS: touch should generate just one event. */
-   {
-@@ -619,6 +616,69 @@ TEST_IMPL(fs_event_watch_file_exact_path) {
-   return 0;
- }
- 
-+static void file_remove_cb(uv_fs_event_t* handle,
-+                           const char* path,
-+                           int events,
-+                           int status) {
-+  fs_event_cb_called++;
-+
-+  expect_filename(path, "file1");
-+  /* TODO(bnoordhuis) Harmonize the behavior across platforms. Right now
-+   * this test merely ensures the status quo doesn't regress.
-+   */
-+#if defined(_AIX) || defined(__linux__)
-+  ASSERT(UV_CHANGE == events);
-+#else
-+  ASSERT(UV_RENAME == events);
-+#endif
-+  ASSERT(0 == status);
-+
-+  uv_close((uv_handle_t*) handle, NULL);
-+}
-+
-+static void file_remove_next(uv_timer_t* handle) {
-+  uv_close((uv_handle_t*) handle, NULL);
-+  remove("watch_dir/file1");
-+}
-+
-+static void file_remove_start(uv_timer_t* handle) {
-+  uv_fs_event_t* watcher;
-+  uv_loop_t* loop;
-+
-+  loop = handle->loop;
-+  watcher = handle->data;
-+
-+  ASSERT(0 == uv_fs_event_init(loop, watcher));
-+  ASSERT(0 == uv_fs_event_start(watcher, file_remove_cb, "watch_dir/file1", 0));
-+  ASSERT(0 == uv_timer_start(handle, file_remove_next, 50, 0));
-+}
-+
-+TEST_IMPL(fs_event_watch_file_remove) {
-+  uv_fs_event_t watcher;
-+  uv_timer_t timer;
-+  uv_loop_t* loop;
-+
-+#if defined(__MVS__)
-+  RETURN_SKIP("test does not work on this OS");
-+#endif
-+
-+  remove("watch_dir/file1");
-+  remove("watch_dir/");
-+  create_dir("watch_dir");
-+  create_file("watch_dir/file1");
-+
-+  loop = uv_default_loop();
-+  timer.data = &watcher;
-+
-+  ASSERT(0 == uv_timer_init(loop, &timer));
-+  ASSERT(0 == uv_timer_start(&timer, file_remove_start, 500, 0));
-+  ASSERT(0 == uv_run(loop, UV_RUN_DEFAULT));
-+  ASSERT(1 == fs_event_cb_called);
-+
-+  MAKE_VALGRIND_HAPPY();
-+  return 0;
-+}
-+
- TEST_IMPL(fs_event_watch_file_twice) {
- #if defined(NO_FS_EVENTS)
-   RETURN_SKIP(NO_FS_EVENTS);
-diff --git a/deps/uv/test/test-list.h b/deps/uv/test/test-list.h
-index f498c7dc81f1e54ce788a1c6a5c7653cbd5bbd31..81c5d9ae142e93d4d0a0a9de510a1166e6564d36 100644
---- a/deps/uv/test/test-list.h
-+++ b/deps/uv/test/test-list.h
-@@ -327,6 +327,7 @@ TEST_DECLARE   (fs_event_watch_dir_short_path)
- #endif
- TEST_DECLARE   (fs_event_watch_file)
- TEST_DECLARE   (fs_event_watch_file_exact_path)
-+TEST_DECLARE   (fs_event_watch_file_remove)
- TEST_DECLARE   (fs_event_watch_file_twice)
- TEST_DECLARE   (fs_event_watch_file_current_dir)
- #ifdef _WIN32
-@@ -903,6 +904,7 @@ TASK_LIST_START
- #endif
-   TEST_ENTRY  (fs_event_watch_file)
-   TEST_ENTRY  (fs_event_watch_file_exact_path)
-+  TEST_ENTRY  (fs_event_watch_file_remove)
-   TEST_ENTRY  (fs_event_watch_file_twice)
-   TEST_ENTRY  (fs_event_watch_file_current_dir)
- #ifdef _WIN32