fsevents: really watch files with fsevents on macos 10.7+

In the original PR, the ifdef conditional was reversed, leading to the old code-path still being used. This also reduces some of the redundancy in the conditional checks, by factoring out the common test. And fixes a divergence in functionality kFSEventsRenamed => kFSEventStreamEventFlagItemRenamed And actually includes the part of the original PR to kqueue that enabled watching files with fsevents! Fixes: libuv#387 PR-URL: libuv#2082 Refs: libuv#1572 Reviewed-By: Ben Noordhuis <[email protected]>
bnoordhuis · Jan 4, 2019 · 2d2af38 · 2d2af38
1 parent 3be96bb
commit 2d2af38
Show file tree

Hide file tree

Showing 6 changed files with 132 additions and 112 deletions.
diff --git a/src/unix/aix.c b/src/unix/aix.c
@@ -733,7 +733,11 @@ int uv_fs_event_start(uv_fs_event_t* handle,
 
 
   /* Figure out whether filename is absolute or not */
-  if (filename[0] == '/') {
+  if (filename[0] == '\0') {
+    /* Missing a pathname */
+    return UV_ENOENT;
+  }
+  else if (filename[0] == '/') {
     /* We have absolute pathname */
     /* TODO(bnoordhuis) Check uv__strscpy() return value. */
     uv__strscpy(absolute_path, filename, sizeof(absolute_path));

diff --git a/src/unix/fsevents.c b/src/unix/fsevents.c
@@ -255,42 +255,55 @@ static void uv__fsevents_event_cb(ConstFSEventStreamRef streamRef,
       path = paths[i];
       len = strlen(path);
 
+      if (handle->realpath_len == 0)
+        continue; /* This should be unreachable */
+
       /* Filter out paths that are outside handle's request */
-      if (strncmp(path, handle->realpath, handle->realpath_len) != 0)
+      if (len < handle->realpath_len)
+        continue;
+
+      if (handle->realpath_len != len &&
+          path[handle->realpath_len] != '/')
+        /* Make sure that realpath actually named a directory,
+         * or that we matched the whole string */
         continue;
 
-      if (handle->realpath_len > 1 || *handle->realpath != '/') {
+      if (memcmp(path, handle->realpath, handle->realpath_len) != 0)
+        continue;
+
+      if (!(handle->realpath_len == 1 && handle->realpath[0] == '/')) {
+        /* Remove common prefix, unless the watched folder is "/" */
         path += handle->realpath_len;
         len -= handle->realpath_len;
 
-        /* Skip forward slash */
-        if (*path != '\0') {
+        /* Ignore events with path equal to directory itself */
+        if (len <= 1 && (flags & kFSEventStreamEventFlagItemIsDir))
+          continue;
+
+        if (len == 0) {
+          /* Since we're using fsevents to watch the file itself,
+           * realpath == path, and we now need to get the basename of the file back
+           * (for commonality with other codepaths and platforms). */
+          while (len < handle->realpath_len && path[-1] != '/') {
+            path--;
+            len++;
+          }
+          /* Created and Removed seem to be always set, but don't make sense */
+          flags &= ~kFSEventsRenamed;
+        } else {
+          /* Skip forward slash */
           path++;
           len--;
         }
       }
 
-#ifdef MAC_OS_X_VERSION_10_7
-      /* Ignore events with path equal to directory itself */
-      if (len == 0)
-        continue;
-#else
-      if (len == 0 && (flags & kFSEventStreamEventFlagItemIsDir))
-        continue;
-#endif /* MAC_OS_X_VERSION_10_7 */
-
       /* Do not emit events from subdirectories (without option set) */
-      if ((handle->cf_flags & UV_FS_EVENT_RECURSIVE) == 0 && *path != 0) {
+      if ((handle->cf_flags & UV_FS_EVENT_RECURSIVE) == 0 && *path != '\0') {
         pos = strchr(path + 1, '/');
         if (pos != NULL)
           continue;
       }
 
-#ifndef MAC_OS_X_VERSION_10_7
-      path = "";
-      len = 0;
-#endif /* MAC_OS_X_VERSION_10_7 */
-
       event = uv__malloc(sizeof(*event) + len);
       if (event == NULL)
         break;
@@ -299,22 +312,11 @@ static void uv__fsevents_event_cb(ConstFSEventStreamRef streamRef,
       memcpy(event->path, path, len + 1);
       event->events = UV_RENAME;
 
-#ifdef MAC_OS_X_VERSION_10_7
-      if (0 != (flags & kFSEventsModified) &&
-          0 == (flags & kFSEventsRenamed)) {
-        event->events = UV_CHANGE;
-      }
-#else
-      if (0 != (flags & kFSEventsModified) &&
-          0 != (flags & kFSEventStreamEventFlagItemIsDir) &&
-          0 == (flags & kFSEventStreamEventFlagItemRenamed)) {
-        event->events = UV_CHANGE;
-      }
-      if (0 == (flags & kFSEventStreamEventFlagItemIsDir) &&
-          0 == (flags & kFSEventStreamEventFlagItemRenamed)) {
-        event->events = UV_CHANGE;
+      if (0 == (flags & kFSEventsRenamed)) {
+        if (0 != (flags & kFSEventsModified) ||
+            0 == (flags & kFSEventStreamEventFlagItemIsDir))
+          event->events = UV_CHANGE;
       }
-#endif /* MAC_OS_X_VERSION_10_7 */
 
       QUEUE_INSERT_TAIL(&head, &event->member);
     }

diff --git a/src/unix/internal.h b/src/unix/internal.h
@@ -284,24 +284,6 @@ int uv__fsevents_init(uv_fs_event_t* handle);
 int uv__fsevents_close(uv_fs_event_t* handle);
 void uv__fsevents_loop_delete(uv_loop_t* loop);
 
-/* OSX < 10.7 has no file events, polyfill them */
-#ifndef MAC_OS_X_VERSION_10_7
-
-static const int kFSEventStreamCreateFlagFileEvents = 0x00000010;
-static const int kFSEventStreamEventFlagItemCreated = 0x00000100;
-static const int kFSEventStreamEventFlagItemRemoved = 0x00000200;
-static const int kFSEventStreamEventFlagItemInodeMetaMod = 0x00000400;
-static const int kFSEventStreamEventFlagItemRenamed = 0x00000800;
-static const int kFSEventStreamEventFlagItemModified = 0x00001000;
-static const int kFSEventStreamEventFlagItemFinderInfoMod = 0x00002000;
-static const int kFSEventStreamEventFlagItemChangeOwner = 0x00004000;
-static const int kFSEventStreamEventFlagItemXattrMod = 0x00008000;
-static const int kFSEventStreamEventFlagItemIsFile = 0x00010000;
-static const int kFSEventStreamEventFlagItemIsDir = 0x00020000;
-static const int kFSEventStreamEventFlagItemIsSymlink = 0x00040000;
-
-#endif /* __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 1070 */
-
 #endif /* defined(__APPLE__) */
 
 UV_UNUSED(static void uv__update_time(uv_loop_t* loop)) {

diff --git a/src/unix/kqueue.c b/src/unix/kqueue.c
@@ -452,79 +452,78 @@ int uv_fs_event_start(uv_fs_event_t* handle,
                       uv_fs_event_cb cb,
                       const char* path,
                       unsigned int flags) {
-#if defined(__APPLE__)
-  struct stat statbuf;
-#endif /* defined(__APPLE__) */
   int fd;
 
   if (uv__is_active(handle))
     return UV_EINVAL;
 
-  /* TODO open asynchronously - but how do we report back errors? */
-  fd = open(path, O_RDONLY);
-  if (fd == -1)
-    return UV__ERR(errno);
-
-  uv__handle_start(handle);
-  uv__io_init(&handle->event_watcher, uv__fs_event, fd);
-  handle->path = uv__strdup(path);
-  handle->cb = cb;
-
 #if defined(__APPLE__)
-  if (uv__has_forked_with_cfrunloop)
-    goto fallback;
-
   /* 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;
-
-  /* The fallback fd is no longer needed */
-  uv__close(fd);
-  handle->event_watcher.fd = -1;
-
-  return uv__fsevents_init(handle);
-
-fallback:
+  if (!uv__has_forked_with_cfrunloop) {
+    int r;
+    /* The fallback fd is not used */
+    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);
+    } else {
+      uv__free(handle->path);
+      handle->path = NULL;
+    }
+    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);
+
+  handle->path = uv__strdup(path);
+  if (handle->path == NULL)
+    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);
 
   return 0;
 }
 
 
 int uv_fs_event_stop(uv_fs_event_t* handle) {
+  int r;
+  r = 0;
+
   if (!uv__is_active(handle))
     return 0;
 
   uv__handle_stop(handle);
 
 #if defined(__APPLE__)
-  if (uv__has_forked_with_cfrunloop || uv__fsevents_close(handle))
-#endif /* defined(__APPLE__) */
-  {
-    uv__io_close(handle->loop, &handle->event_watcher);
-  }
-
-  uv__free(handle->path);
-  handle->path = NULL;
+  if (!uv__has_forked_with_cfrunloop)
+    r = uv__fsevents_close(handle);
+#endif
 
   if (handle->event_watcher.fd != -1) {
-    /* When FSEvents is used, we don't use the event_watcher's fd under certain
-     * confitions. (see uv_fs_event_start) */
+    uv__io_close(handle->loop, &handle->event_watcher);
     uv__close(handle->event_watcher.fd);
     handle->event_watcher.fd = -1;
   }
 
-  return 0;
+  uv__free(handle->path);
+  handle->path = NULL;
+
+  return r;
 }
 
 

diff --git a/test/test-fs-event.c b/test/test-fs-event.c
@@ -576,6 +576,14 @@ TEST_IMPL(fs_event_watch_file_exact_path) {
   create_dir("watch_dir");
   create_file("watch_dir/file.js");
   create_file("watch_dir/file.jsx");
+#if defined(__APPLE__) && !defined(MAC_OS_X_VERSION_10_12)
+  /* Empirically, FSEvents seems to (reliably) report the preceeding
+   * create_file events prior to macOS 10.11.6 in the subsequent fs_watch
+   * creation, but that behavior hasn't been observed to occur on newer
+   * versions. Give a long delay here to let the system settle before running
+   * the test. */
+  uv_sleep(1100);
+#endif
 
   r = uv_fs_event_init(loop, &fs_event);
   ASSERT(r == 0);
@@ -648,7 +656,7 @@ TEST_IMPL(fs_event_watch_file_current_dir) {
   r = uv_timer_init(loop, &timer);
   ASSERT(r == 0);
 
-  r = uv_timer_start(&timer, timer_cb_touch, 100, 0);
+  r = uv_timer_start(&timer, timer_cb_touch, 1100, 0);
   ASSERT(r == 0);
 
   ASSERT(timer_cb_touch_called == 0);
@@ -936,32 +944,48 @@ TEST_IMPL(fs_event_getpath) {
   RETURN_SKIP(NO_FS_EVENTS);
 #endif
   uv_loop_t* loop = uv_default_loop();
+  unsigned i;
   int r;
   char buf[1024];
   size_t len;
+  const char* const watch_dir[] = {
+    "watch_dir",
+    "watch_dir/",
+    "watch_dir///",
+    "watch_dir/subfolder/..",
+    "watch_dir//subfolder//..//",
+  };
 
   create_dir("watch_dir");
+  create_dir("watch_dir/subfolder");
 
-  r = uv_fs_event_init(loop, &fs_event);
-  ASSERT(r == 0);
-  len = sizeof buf;
-  r = uv_fs_event_getpath(&fs_event, buf, &len);
-  ASSERT(r == UV_EINVAL);
-  r = uv_fs_event_start(&fs_event, fail_cb, "watch_dir", 0);
-  ASSERT(r == 0);
-  len = sizeof buf;
-  r = uv_fs_event_getpath(&fs_event, buf, &len);
-  ASSERT(r == 0);
-  ASSERT(buf[len - 1] != 0);
-  ASSERT(buf[len] == '\0');
-  ASSERT(memcmp(buf, "watch_dir", len) == 0);
-  r = uv_fs_event_stop(&fs_event);
-  ASSERT(r == 0);
-  uv_close((uv_handle_t*) &fs_event, close_cb);
 
-  uv_run(loop, UV_RUN_DEFAULT);
+  for (i = 0; i < ARRAY_SIZE(watch_dir); i++) {
+    r = uv_fs_event_init(loop, &fs_event);
+    ASSERT(r == 0);
+    len = sizeof buf;
+    r = uv_fs_event_getpath(&fs_event, buf, &len);
+    ASSERT(r == UV_EINVAL);
+    r = uv_fs_event_start(&fs_event, fail_cb, watch_dir[i], 0);
+    ASSERT(r == 0);
+    len = 0;
+    r = uv_fs_event_getpath(&fs_event, buf, &len);
+    ASSERT(r == UV_ENOBUFS);
+    ASSERT(len < sizeof buf); /* sanity check */
+    ASSERT(len == strlen(watch_dir[i]) + 1);
+    r = uv_fs_event_getpath(&fs_event, buf, &len);
+    ASSERT(r == 0);
+    ASSERT(len == strlen(watch_dir[i]));
+    ASSERT(strcmp(buf, watch_dir[i]) == 0);
+    r = uv_fs_event_stop(&fs_event);
+    ASSERT(r == 0);
+    uv_close((uv_handle_t*) &fs_event, close_cb);
 
-  ASSERT(close_cb_called == 1);
+    uv_run(loop, UV_RUN_DEFAULT);
+
+    ASSERT(close_cb_called == 1);
+    close_cb_called = 0;
+  }
 
   remove("watch_dir/");
   MAKE_VALGRIND_HAPPY();
@@ -1082,6 +1106,9 @@ TEST_IMPL(fs_event_watch_invalid_path) {
   r = uv_fs_event_start(&fs_event, fs_event_cb_file, "<:;", 0);
   ASSERT(r != 0);
   ASSERT(uv_is_active((uv_handle_t*) &fs_event) == 0);
+  r = uv_fs_event_start(&fs_event, fs_event_cb_file, "", 0);
+  ASSERT(r != 0);
+  ASSERT(uv_is_active((uv_handle_t*) &fs_event) == 0);
   MAKE_VALGRIND_HAPPY();
   return 0;
 }
diff --git a/test/test-list.h b/test/test-list.h
@@ -436,9 +436,13 @@ TEST_DECLARE  (fork_socketpair)
 TEST_DECLARE  (fork_socketpair_started)
 TEST_DECLARE  (fork_signal_to_child)
 TEST_DECLARE  (fork_signal_to_child_closed)
+#ifndef __APPLE__ /* This is forbidden in a fork child: The process has forked
+                     and you cannot use this CoreFoundation functionality
+                     safely. You MUST exec(). */
 TEST_DECLARE  (fork_fs_events_child)
 TEST_DECLARE  (fork_fs_events_child_dir)
 TEST_DECLARE  (fork_fs_events_file_parent_child)
+#endif
 #ifndef __MVS__
 TEST_DECLARE  (fork_threadpool_queue_work_simple)
 #endif
@@ -945,9 +949,11 @@ TASK_LIST_START
   TEST_ENTRY  (fork_socketpair_started)
   TEST_ENTRY  (fork_signal_to_child)
   TEST_ENTRY  (fork_signal_to_child_closed)
+#ifndef __APPLE__
   TEST_ENTRY  (fork_fs_events_child)
   TEST_ENTRY  (fork_fs_events_child_dir)
   TEST_ENTRY  (fork_fs_events_file_parent_child)
+#endif
 #ifndef __MVS__
   TEST_ENTRY  (fork_threadpool_queue_work_simple)
 #endif