From 558427a1818935d752dd9d8c9e2cae39ed85ce89 Mon Sep 17 00:00:00 2001 From: Tyler Ang-Wanek Date: Fri, 21 Jun 2019 10:16:45 -0700 Subject: [PATCH 1/2] Make event queue shared between nsfw and native implementation Don't create an EventBaton which can be leaked according to the documentation in uv_async_send. --- includes/NSFW.h | 15 ++++----- includes/NativeInterface.h | 4 +-- src/NSFW.cpp | 62 ++++++++++++++------------------------ src/NativeInterface.cpp | 9 ++---- 4 files changed, 31 insertions(+), 59 deletions(-) diff --git a/includes/NSFW.h b/includes/NSFW.h index 58934418..9c466722 100644 --- a/includes/NSFW.h +++ b/includes/NSFW.h @@ -1,6 +1,7 @@ #ifndef NSFW_H #define NSFW_H +#include "Queue.h" #include "NativeInterface.h" #include #include @@ -13,12 +14,15 @@ class NSFW : public ObjectWrap { public: static NAN_MODULE_INIT(Init); - static void cleanupEventCallback(void *arg); static void fireErrorCallback(uv_async_t *handle); static void fireEventCallback(uv_async_t *handle); static void pollForEvents(void *arg); Persistent mPersistentHandle; +private: + NSFW(uint32_t debounceMS, std::string path, Callback *eventCallback, Callback *errorCallback); + ~NSFW(); + uint32_t mDebounceMS; uv_async_t mErrorCallbackAsync; uv_async_t mEventCallbackAsync; @@ -30,20 +34,13 @@ class NSFW : public ObjectWrap { std::string mPath; uv_thread_t mPollThread; std::atomic mRunning; -private: - NSFW(uint32_t debounceMS, std::string path, Callback *eventCallback, Callback *errorCallback); - ~NSFW(); + std::shared_ptr mQueue; struct ErrorBaton { NSFW *nsfw; std::string error; }; - struct EventBaton { - NSFW *nsfw; - std::unique_ptr>> events; - }; - static NAN_METHOD(JSNew); static NAN_METHOD(Start); diff --git a/includes/NativeInterface.h b/includes/NativeInterface.h index e2322d7c..35d64ada 100644 --- a/includes/NativeInterface.h +++ b/includes/NativeInterface.h @@ -17,16 +17,14 @@ using NativeImplementation = InotifyService; class NativeInterface { public: - NativeInterface(const std::string &path); + NativeInterface(const std::string &path, std::shared_ptr queue); ~NativeInterface(); std::string getError(); - std::unique_ptr>> getEvents(); bool hasErrored(); bool isWatching(); private: - std::shared_ptr mQueue; std::unique_ptr mNativeInterface; }; diff --git a/src/NSFW.cpp b/src/NSFW.cpp index 13eb8a65..a9f330d5 100644 --- a/src/NSFW.cpp +++ b/src/NSFW.cpp @@ -18,7 +18,9 @@ NSFW::NSFW(uint32_t debounceMS, std::string path, Callback *eventCallback, Callb mInterface(NULL), mInterfaceLockValid(false), mPath(path), - mRunning(false) { + mRunning(false), + mQueue(std::make_shared()) + { HandleScope scope; v8::Local obj = New(); mPersistentHandle.Reset(obj); @@ -37,11 +39,6 @@ NSFW::~NSFW() { } } -void NSFW::cleanupEventCallback(void *arg) { - EventBaton *baton = (EventBaton *)arg; - delete baton; -} - void NSFW::fireErrorCallback(uv_async_t *handle) { Nan::HandleScope scope; ErrorBaton *baton = (ErrorBaton *)handle->data; @@ -54,33 +51,27 @@ void NSFW::fireErrorCallback(uv_async_t *handle) { void NSFW::fireEventCallback(uv_async_t *handle) { Nan::HandleScope scope; - EventBaton *baton = (EventBaton *)handle->data; - if (baton->events->empty()) { - uv_thread_t cleanup; - uv_thread_create(&cleanup, NSFW::cleanupEventCallback, baton); - - #if defined(__APPLE_CC__) || defined(__linux__) || defined(__FreeBSD__) - pthread_detach(cleanup); - #endif - + NSFW *nsfw = (NSFW *)handle->data; + auto events = nsfw->mQueue->dequeueAll(); + if (events == nullptr) { return; } - v8::Local eventArray = New((int)baton->events->size()); + v8::Local eventArray = New((int)events->size()); - for (unsigned int i = 0; i < baton->events->size(); ++i) { + for (unsigned int i = 0; i < events->size(); ++i) { v8::Local jsEvent = New(); - jsEvent->Set(New("action").ToLocalChecked(), New((*baton->events)[i]->type)); - jsEvent->Set(New("directory").ToLocalChecked(), New((*baton->events)[i]->fromDirectory).ToLocalChecked()); + jsEvent->Set(New("action").ToLocalChecked(), New((*events)[i]->type)); + jsEvent->Set(New("directory").ToLocalChecked(), New((*events)[i]->fromDirectory).ToLocalChecked()); - if ((*baton->events)[i]->type == RENAMED) { - jsEvent->Set(New("oldFile").ToLocalChecked(), New((*baton->events)[i]->fromFile).ToLocalChecked()); - jsEvent->Set(New("newDirectory").ToLocalChecked(), New((*baton->events)[i]->toDirectory).ToLocalChecked()); - jsEvent->Set(New("newFile").ToLocalChecked(), New((*baton->events)[i]->toFile).ToLocalChecked()); + if ((*events)[i]->type == RENAMED) { + jsEvent->Set(New("oldFile").ToLocalChecked(), New((*events)[i]->fromFile).ToLocalChecked()); + jsEvent->Set(New("newDirectory").ToLocalChecked(), New((*events)[i]->toDirectory).ToLocalChecked()); + jsEvent->Set(New("newFile").ToLocalChecked(), New((*events)[i]->toFile).ToLocalChecked()); } else { - jsEvent->Set(New("file").ToLocalChecked(), New((*baton->events)[i]->fromFile).ToLocalChecked()); + jsEvent->Set(New("file").ToLocalChecked(), New((*events)[i]->fromFile).ToLocalChecked()); } eventArray->Set(i, jsEvent); @@ -90,14 +81,7 @@ void NSFW::fireEventCallback(uv_async_t *handle) { eventArray }; - baton->nsfw->mEventCallback->Call(1, argv); - - uv_thread_t cleanup; - uv_thread_create(&cleanup, NSFW::cleanupEventCallback, baton); - - #if defined(__APPLE_CC__) || defined(__linux__) || defined(__FreeBSD__) - pthread_detach(cleanup); - #endif + nsfw->mEventCallback->Call(1, argv); } void NSFW::pollForEvents(void *arg) { @@ -116,18 +100,14 @@ void NSFW::pollForEvents(void *arg) { uv_mutex_unlock(&nsfw->mInterfaceLock); break; } - auto events = nsfw->mInterface->getEvents(); - if (events == NULL) { + + if (nsfw->mQueue->count() == 0) { uv_mutex_unlock(&nsfw->mInterfaceLock); sleep_for_ms(50); continue; } - EventBaton *baton = new EventBaton; - baton->nsfw = nsfw; - baton->events = std::move(events); - - nsfw->mEventCallbackAsync.data = (void *)baton; + nsfw->mEventCallbackAsync.data = (void *)nsfw; uv_async_send(&nsfw->mEventCallbackAsync); uv_mutex_unlock(&nsfw->mInterfaceLock); @@ -230,7 +210,8 @@ void NSFW::StartWorker::Execute() { return; } - mNSFW->mInterface = new NativeInterface(mNSFW->mPath); + mNSFW->mQueue->clear(); + mNSFW->mInterface = new NativeInterface(mNSFW->mPath, mNSFW->mQueue); if (mNSFW->mInterface->isWatching()) { mNSFW->mRunning = true; uv_thread_create(&mNSFW->mPollThread, NSFW::pollForEvents, mNSFW); @@ -306,6 +287,7 @@ void NSFW::StopWorker::Execute() { uv_mutex_lock(&mNSFW->mInterfaceLock); delete mNSFW->mInterface; mNSFW->mInterface = NULL; + mNSFW->mQueue->clear(); uv_mutex_unlock(&mNSFW->mInterfaceLock); } diff --git a/src/NativeInterface.cpp b/src/NativeInterface.cpp index ee1d8dce..3af66e8e 100644 --- a/src/NativeInterface.cpp +++ b/src/NativeInterface.cpp @@ -1,8 +1,7 @@ #include "../includes/NativeInterface.h" -NativeInterface::NativeInterface(const std::string &path) { - mQueue = std::make_shared(); - mNativeInterface.reset(new NativeImplementation(mQueue, path)); +NativeInterface::NativeInterface(const std::string &path, std::shared_ptr queue) { + mNativeInterface.reset(new NativeImplementation(queue, path)); } NativeInterface::~NativeInterface() { @@ -13,10 +12,6 @@ std::string NativeInterface::getError() { return mNativeInterface->getError(); } -std::unique_ptr>> NativeInterface::getEvents() { - return mQueue->dequeueAll(); -} - bool NativeInterface::hasErrored() { return mNativeInterface->hasErrored(); } From 83c97b4dca1a2f94b8a9d5e2f6a338d2c76ed88b Mon Sep 17 00:00:00 2001 From: Tyler Ang-Wanek Date: Fri, 21 Jun 2019 11:56:36 -0700 Subject: [PATCH 2/2] Add timeout between mkdir steps to increase test stability This is not ideal. We need to track down why these events are occasionally being skipped. --- js/spec/index-spec.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/js/spec/index-spec.js b/js/spec/index-spec.js index 3d5103bd..8199d47d 100644 --- a/js/spec/index-spec.js +++ b/js/spec/index-spec.js @@ -2,12 +2,15 @@ const nsfw = require('../src/'); const path = require('path'); const fse = require('fs-extra'); const exec = require('executive'); +const { promisify } = require('util'); jasmine.DEFAULT_TIMEOUT_INTERVAL = 120000; const DEBOUNCE = 1000; const TIMEOUT_PER_STEP = 3000; +const timeout = promisify(setTimeout); + describe('Node Sentinel File Watcher', function() { const workDir = path.resolve('./mockfs'); @@ -396,7 +399,9 @@ describe('Node Sentinel File Watcher', function() { return paths.reduce((chain, dir) => { directory = path.join(directory, dir); const nextDirectory = directory; - return chain.then(() => fse.mkdir(nextDirectory)); + return chain + .then(() => fse.mkdir(nextDirectory)) + .then(() => timeout(60)); }, Promise.resolve()); }) .then(() => fse.open(path.join(directory, file), 'w'))