Move all run-loop calls into the run-loop thread, and use correct memory handling (retain/release).

runtime/bin/file_system_watcher_macos.cc

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "platform/globals.h"
 #if defined(TARGET_OS_MACOS)
 
 #include "bin/file_system_watcher.h"
 
 #include <errno.h>  // NOLINT
@@ skipping 37 matching lines @@
     uint32_t flags;
     char path[PATH_MAX];
   } data;
   uint8_t bytes[PATH_MAX + 8];
 };
 
 class FSEventsWatcher {
  public:
   class Node {
    public:
-    Node(FSEventsWatcher* watcher, intptr_t base_path_length, int read_fd,
+    Node(FSEventsWatcher* watcher, char* base_path, int read_fd,
          int write_fd, bool recursive)
         : watcher_(watcher),
           ready_(false),
-          base_path_length_(base_path_length),
+          base_path_length_(strlen(base_path)),
+          path_ref_(CFStringCreateWithCString(
+              NULL, base_path, kCFStringEncodingUTF8)),
           read_fd_(read_fd),
           write_fd_(write_fd),
           recursive_(recursive),
-          ref_(NULL) {}
+          ref_(NULL) {

[Søren Gjesse, 2014/02/07 10:20:02]

Please keep this code in the Start method and call that after creating the
object.

[Anders Johnsen, 2014/02/07 12:49:24]

Done.

+      CFRunLoopTimerContext context;
+      memset(&context, 0, sizeof(context));
+      context.info = this;

[Søren Gjesse, 2014/02/07 10:20:02]

Please add a comment that this is scheduling StartCallback in the run loop
thread.

[Anders Johnsen, 2014/02/07 12:49:24]

Done.

+      CFRunLoopTimerRef timer = CFRunLoopTimerCreate(
+          NULL, 0, 0, 0, 0, Node::StartCallback, &context);
+      CFRunLoopAddTimer(watcher_->run_loop_, timer, kCFRunLoopCommonModes);
+      CFRelease(timer);
+      watcher_->monitor_.Enter();
+      while (!ready_) {
+        watcher_->monitor_.Wait(Monitor::kNoTimeout);
+      }
+      watcher_->monitor_.Exit();
+    }
 
     ~Node() {

[Søren Gjesse, 2014/02/07 10:20:02]

I am not a great fan of placing this code in the destructor. I think it is much
better to have it in Stop, and then delete the object after calling Stop.

[Anders Johnsen, 2014/02/07 12:49:24]

Done.

+      ASSERT(ready_);
+      CFRunLoopTimerContext context;
+      memset(&context, 0, sizeof(context));
+      context.info = this;

[Søren Gjesse, 2014/02/07 10:20:02]

Please add a comment that this is scheduling StopCallback in the run loop
thread.

[Anders Johnsen, 2014/02/07 12:49:24]

Done.

+      CFRunLoopTimerRef timer = CFRunLoopTimerCreate(
+          NULL, 0, 0, 0, 0, StopCallback, &context);
+      CFRunLoopAddTimer(watcher_->run_loop_, timer, kCFRunLoopCommonModes);
+      watcher_->monitor_.Enter();
+      while (ready_) {
+        watcher_->monitor_.Wait(Monitor::kNoTimeout);
+      }
+      watcher_->monitor_.Exit();
       close(write_fd_);
+      CFRelease(path_ref_);
     }
 
     void set_ref(FSEventStreamRef ref) {
       ref_ = ref;
     }
 
     static void StartCallback(CFRunLoopTimerRef timer, void* info) {
       Node* node = reinterpret_cast<Node*>(info);
+
+      FSEventStreamContext context;
+      context.version = 0;
+      context.info = reinterpret_cast<void*>(node);
+      context.retain = NULL;
+      context.release = NULL;
+      context.copyDescription = NULL;
+      CFArrayRef array = CFArrayCreate(
+          NULL, reinterpret_cast<const void**>(&node->path_ref_), 1, NULL);
+      FSEventStreamRef ref = FSEventStreamCreate(
+          NULL,
+          Callback,
+          &context,
+          array,
+          kFSEventStreamEventIdSinceNow,
+          0.10,
+          kFSEventStreamCreateFlagNoDefer | kFSEventStreamCreateFlagFileEvents);
+      CFRelease(array);
+
+      node->set_ref(ref);
+
       FSEventStreamScheduleWithRunLoop(
           node->ref_,
           node->watcher_->run_loop_,
           kCFRunLoopDefaultMode);
+
       FSEventStreamStart(node->ref_);
       FSEventStreamFlushSync(node->ref_);
+
       node->watcher_->monitor_.Enter();
       node->ready_ = true;
       node->watcher_->monitor_.Notify();
       node->watcher_->monitor_.Exit();
     }
 
-    void Start() {
-      ASSERT(!ready_);
-      CFRunLoopTimerContext context;
-      memset(&context, 0, sizeof(context));
-      context.info = this;
-      CFRunLoopTimerRef timer = CFRunLoopTimerCreate(
-          NULL, 0, 0, 0, 0, StartCallback, &context);
-      CFRunLoopAddTimer(watcher_->run_loop_, timer, kCFRunLoopCommonModes);
-      watcher_->monitor_.Enter();
-      while (!ready_) {
-        watcher_->monitor_.Wait(Monitor::kNoTimeout);
-      }
-      watcher_->monitor_.Exit();
-    }
-
     static void StopCallback(CFRunLoopTimerRef timer, void* info) {
       Node* node = reinterpret_cast<Node*>(info);
       FSEventStreamStop(node->ref_);
       FSEventStreamInvalidate(node->ref_);
       FSEventStreamRelease(node->ref_);
       node->watcher_->monitor_.Enter();
       node->ready_ = false;
       node->watcher_->monitor_.Notify();
       node->watcher_->monitor_.Exit();
     }
 
-    void Stop() {
-      ASSERT(ready_);
-      CFRunLoopTimerContext context;
-      memset(&context, 0, sizeof(context));
-      context.info = this;
-      CFRunLoopTimerRef timer = CFRunLoopTimerCreate(
-          NULL, 0, 0, 0, 0, StopCallback, &context);
-      CFRunLoopAddTimer(watcher_->run_loop_, timer, kCFRunLoopCommonModes);
-      watcher_->monitor_.Enter();
-      while (ready_) {
-        watcher_->monitor_.Wait(Monitor::kNoTimeout);
-      }
-      watcher_->monitor_.Exit();
-    }
-
     bool ready() const { return ready_; }
     intptr_t base_path_length() const { return base_path_length_; }
     int read_fd() const { return read_fd_; }
     int write_fd() const { return write_fd_; }
     bool recursive() const { return recursive_; }
 
    private:
     FSEventsWatcher* watcher_;
     bool ready_;
     intptr_t base_path_length_;
+    CFStringRef path_ref_;
     int read_fd_;
     int write_fd_;
     bool recursive_;
     FSEventStreamRef ref_;
   };
 
   FSEventsWatcher() : run_loop_(0) {

[Søren Gjesse, 2014/02/07 10:20:02]

Please add a Start method for starting the run-loop thread and call that after
constructing the object.

[Anders Johnsen, 2014/02/07 12:49:24]

Done.

     Thread::Start(Run, reinterpret_cast<uword>(this));
   }
 
+  static void StopCallback(CFRunLoopTimerRef timer, void* info) {
+    FSEventsWatcher* watcher = reinterpret_cast<FSEventsWatcher*>(info);
+    CFRunLoopStop(watcher->run_loop_);
+    CFRelease(watcher->run_loop_);
+    watcher->monitor_.Enter();
+    watcher->run_loop_ = NULL;
+    watcher->monitor_.Notify();
+    watcher->monitor_.Exit();
+  }
+
   ~FSEventsWatcher() {

[Søren Gjesse, 2014/02/07 10:20:02]

Please move this to a Stop method and call that before calling the destructor.

[Anders Johnsen, 2014/02/07 12:49:24]

Done.

-    CFRunLoopStop(run_loop_);
+    CFRunLoopTimerContext context;
+    memset(&context, 0, sizeof(context));
+    context.info = this;
+    CFRunLoopTimerRef timer = CFRunLoopTimerCreate(
+        NULL, 0, 0, 0, 0, StopCallback, &context);
+    CFRunLoopAddTimer(run_loop_, timer, kCFRunLoopCommonModes);
+    CFRelease(timer);
+    monitor_.Enter();
+    while (run_loop_ != NULL) {
+      monitor_.Wait(Monitor::kNoTimeout);
+    }
+    monitor_.Exit();
   }
 
   Monitor& monitor() { return monitor_; }
 
   bool has_run_loop() const { return run_loop_ != NULL; }
 
   static void TimerCallback(CFRunLoopTimerRef timer, void* context) {
     // Dummy callback to keep RunLoop alive.
   }
 
   static void Run(uword arg) {

[Søren Gjesse, 2014/02/07 10:20:02]

We should consider in debug mode to get the thread id here, and then assert that
the different methods run on the expected thread. That would also serve as
documentation of how this works, as it is pretty subtle.

[Anders Johnsen, 2014/02/07 12:49:24]

Done.

     FSEventsWatcher* watcher = reinterpret_cast<FSEventsWatcher*>(arg);
     watcher->run_loop_ = CFRunLoopGetCurrent();
+    CFRetain(watcher->run_loop_);
 
     // Notify, as the run-loop is set.
     watcher->monitor().Enter();
     watcher->monitor().Notify();
     watcher->monitor().Exit();
 
     CFRunLoopTimerRef timer = CFRunLoopTimerCreate(
         NULL,
         CFAbsoluteTimeGetCurrent() + 1,
         1,
         0,
         0,
         TimerCallback,
         NULL);
-
     CFRunLoopAddTimer(watcher->run_loop_, timer, kCFRunLoopCommonModes);
+    CFRelease(timer);
 
     CFRunLoopRun();
   }
 
   Node* AddPath(const char* path, int events, bool recursive) {
     int fds[2];
     VOID_TEMP_FAILURE_RETRY(pipe(fds));
     Socket::SetNonBlocking(fds[0]);
     Socket::SetBlocking(fds[1]);
 
     char base_path[PATH_MAX];
     realpath(path, base_path);
-    CFStringRef path_ref = CFStringCreateWithCString(
-        NULL, base_path, kCFStringEncodingUTF8);
 
-    Node* node = new Node(this, strlen(base_path), fds[0], fds[1], recursive);
-
-    FSEventStreamContext context;
-    context.version = 0;
-    context.info = reinterpret_cast<void*>(node);
-    context.retain = NULL;
-    context.release = NULL;
-    context.copyDescription = NULL;
-    FSEventStreamRef ref = FSEventStreamCreate(
-        NULL,
-        Callback,
-        &context,
-        CFArrayCreate(NULL, reinterpret_cast<const void**>(&path_ref), 1, NULL),
-        kFSEventStreamEventIdSinceNow,
-        0.10,
-        kFSEventStreamCreateFlagNoDefer | kFSEventStreamCreateFlagFileEvents);
-
-    node->set_ref(ref);
-
-    return node;
+    return new Node(this, base_path, fds[0], fds[1], recursive);
   }
 
  private:
   static void Callback(ConstFSEventStreamRef ref,
                        void* client,
                        size_t num_events,
                        void* event_paths,
                        const FSEventStreamEventFlags event_flags[],
                        const FSEventStreamEventId event_ids[]) {
     Node* node = reinterpret_cast<Node*>(client);
@@ skipping 41 matching lines @@
   delete watcher;
 }
 
 
 intptr_t FileSystemWatcher::WatchPath(intptr_t id,
                                       const char* path,
                                       int events,
                                       bool recursive) {
   FSEventsWatcher* watcher = reinterpret_cast<FSEventsWatcher*>(id);
   FSEventsWatcher::Node* node = watcher->AddPath(path, events, recursive);
-  node->Start();
   return reinterpret_cast<intptr_t>(node);
 }
 
 
 void FileSystemWatcher::UnwatchPath(intptr_t id, intptr_t path_id) {
   USE(id);
   FSEventsWatcher::Node* node =
       reinterpret_cast<FSEventsWatcher::Node*>(path_id);
-  node->Stop();
   delete node;
 }
 
 
 intptr_t FileSystemWatcher::GetSocketId(intptr_t id, intptr_t path_id) {
   return reinterpret_cast<FSEventsWatcher::Node*>(path_id)->read_fd();
 }
 
 
 Dart_Handle FileSystemWatcher::ReadEvents(intptr_t id, intptr_t path_id) {
@@ skipping 42 matching lines @@
   }
   return events;
 }
 
 }  // namespace bin
 }  // namespace dart
 
 #endif  // defined(TARGET_OS_MACOS)