Skip to content

Commit 2e9bf2e

Browse files
implausibleimplausible
authored
Merge pull request #77 from Axosoft/fix/memory-leak-with-async-handle
Make event queue shared between nsfw and native implementation
2 parents f50e945 + 83c97b4 commit 2e9bf2e

File tree

5 files changed

+37
-60
lines changed

5 files changed

+37
-60
lines changed

‎includes/NSFW.h

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#ifndef NSFW_H
22
#define NSFW_H
33

4+
#include "Queue.h"
45
#include "NativeInterface.h"
56
#include <nan.h>
67
#include <uv.h>
@@ -13,12 +14,15 @@ class NSFW : public ObjectWrap {
1314
public:
1415
static NAN_MODULE_INIT(Init);
1516

16-
static void cleanupEventCallback(void *arg);
1717
static void fireErrorCallback(uv_async_t *handle);
1818
static void fireEventCallback(uv_async_t *handle);
1919
static void pollForEvents(void *arg);
2020

2121
Persistent<v8::Object> mPersistentHandle;
22+
private:
23+
NSFW(uint32_t debounceMS, std::string path, Callback *eventCallback, Callback *errorCallback);
24+
~NSFW();
25+
2226
uint32_t mDebounceMS;
2327
uv_async_t mErrorCallbackAsync;
2428
uv_async_t mEventCallbackAsync;
@@ -30,20 +34,13 @@ class NSFW : public ObjectWrap {
3034
std::string mPath;
3135
uv_thread_t mPollThread;
3236
std::atomic<bool> mRunning;
33-
private:
34-
NSFW(uint32_t debounceMS, std::string path, Callback *eventCallback, Callback *errorCallback);
35-
~NSFW();
37+
std::shared_ptr<EventQueue> mQueue;
3638

3739
struct ErrorBaton {
3840
NSFW *nsfw;
3941
std::string error;
4042
};
4143

42-
struct EventBaton {
43-
NSFW *nsfw;
44-
std::unique_ptr<std::vector<std::unique_ptr<Event>>> events;
45-
};
46-
4744
static NAN_METHOD(JSNew);
4845

4946
static NAN_METHOD(Start);

‎includes/NativeInterface.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,16 +17,14 @@ using NativeImplementation = InotifyService;
1717

1818
class NativeInterface {
1919
public:
20-
NativeInterface(const std::string &path);
20+
NativeInterface(const std::string &path, std::shared_ptr<EventQueue> queue);
2121
~NativeInterface();
2222

2323
std::string getError();
24-
std::unique_ptr<std::vector<std::unique_ptr<Event>>> getEvents();
2524
bool hasErrored();
2625
bool isWatching();
2726

2827
private:
29-
std::shared_ptr<EventQueue> mQueue;
3028
std::unique_ptr<NativeImplementation> mNativeInterface;
3129
};
3230

‎js/spec/index-spec.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,15 @@ const nsfw = require('../src/');
22
const path = require('path');
33
const fse = require('fs-extra');
44
const exec = require('executive');
5+
const { promisify } = require('util');
56

67
jasmine.DEFAULT_TIMEOUT_INTERVAL = 120000;
78

89
const DEBOUNCE = 1000;
910
const TIMEOUT_PER_STEP = 3000;
1011

12+
const timeout = promisify(setTimeout);
13+
1114
describe('Node Sentinel File Watcher', function() {
1215
const workDir = path.resolve('./mockfs');
1316

@@ -396,7 +399,9 @@ describe('Node Sentinel File Watcher', function() {
396399
return paths.reduce((chain, dir) => {
397400
directory = path.join(directory, dir);
398401
const nextDirectory = directory;
399-
return chain.then(() => fse.mkdir(nextDirectory));
402+
return chain
403+
.then(() => fse.mkdir(nextDirectory))
404+
.then(() => timeout(60));
400405
}, Promise.resolve());
401406
})
402407
.then(() => fse.open(path.join(directory, file), 'w'))

‎src/NSFW.cpp

Lines changed: 22 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@ NSFW::NSFW(uint32_t debounceMS, std::string path, Callback *eventCallback, Callb
1818
mInterface(NULL),
1919
mInterfaceLockValid(false),
2020
mPath(path),
21-
mRunning(false) {
21+
mRunning(false),
22+
mQueue(std::make_shared<EventQueue>())
23+
{
2224
HandleScope scope;
2325
v8::Local<v8::Object> obj = New<v8::Object>();
2426
mPersistentHandle.Reset(obj);
@@ -37,11 +39,6 @@ NSFW::~NSFW() {
3739
}
3840
}
3941

40-
void NSFW::cleanupEventCallback(void *arg) {
41-
EventBaton *baton = (EventBaton *)arg;
42-
delete baton;
43-
}
44-
4542
void NSFW::fireErrorCallback(uv_async_t *handle) {
4643
Nan::HandleScope scope;
4744
ErrorBaton *baton = (ErrorBaton *)handle->data;
@@ -54,33 +51,27 @@ void NSFW::fireErrorCallback(uv_async_t *handle) {
5451

5552
void NSFW::fireEventCallback(uv_async_t *handle) {
5653
Nan::HandleScope scope;
57-
EventBaton *baton = (EventBaton *)handle->data;
58-
if (baton->events->empty()) {
59-
uv_thread_t cleanup;
60-
uv_thread_create(&cleanup, NSFW::cleanupEventCallback, baton);
61-
62-
#if defined(__APPLE_CC__) || defined(__linux__) || defined(__FreeBSD__)
63-
pthread_detach(cleanup);
64-
#endif
65-
54+
NSFW *nsfw = (NSFW *)handle->data;
55+
auto events = nsfw->mQueue->dequeueAll();
56+
if (events == nullptr) {
6657
return;
6758
}
6859

69-
v8::Local<v8::Array> eventArray = New<v8::Array>((int)baton->events->size());
60+
v8::Local<v8::Array> eventArray = New<v8::Array>((int)events->size());
7061

71-
for (unsigned int i = 0; i < baton->events->size(); ++i) {
62+
for (unsigned int i = 0; i < events->size(); ++i) {
7263
v8::Local<v8::Object> jsEvent = New<v8::Object>();
7364

7465

75-
jsEvent->Set(New<v8::String>("action").ToLocalChecked(), New<v8::Number>((*baton->events)[i]->type));
76-
jsEvent->Set(New<v8::String>("directory").ToLocalChecked(), New<v8::String>((*baton->events)[i]->fromDirectory).ToLocalChecked());
66+
jsEvent->Set(New<v8::String>("action").ToLocalChecked(), New<v8::Number>((*events)[i]->type));
67+
jsEvent->Set(New<v8::String>("directory").ToLocalChecked(), New<v8::String>((*events)[i]->fromDirectory).ToLocalChecked());
7768

78-
if ((*baton->events)[i]->type == RENAMED) {
79-
jsEvent->Set(New<v8::String>("oldFile").ToLocalChecked(), New<v8::String>((*baton->events)[i]->fromFile).ToLocalChecked());
80-
jsEvent->Set(New<v8::String>("newDirectory").ToLocalChecked(), New<v8::String>((*baton->events)[i]->toDirectory).ToLocalChecked());
81-
jsEvent->Set(New<v8::String>("newFile").ToLocalChecked(), New<v8::String>((*baton->events)[i]->toFile).ToLocalChecked());
69+
if ((*events)[i]->type == RENAMED) {
70+
jsEvent->Set(New<v8::String>("oldFile").ToLocalChecked(), New<v8::String>((*events)[i]->fromFile).ToLocalChecked());
71+
jsEvent->Set(New<v8::String>("newDirectory").ToLocalChecked(), New<v8::String>((*events)[i]->toDirectory).ToLocalChecked());
72+
jsEvent->Set(New<v8::String>("newFile").ToLocalChecked(), New<v8::String>((*events)[i]->toFile).ToLocalChecked());
8273
} else {
83-
jsEvent->Set(New<v8::String>("file").ToLocalChecked(), New<v8::String>((*baton->events)[i]->fromFile).ToLocalChecked());
74+
jsEvent->Set(New<v8::String>("file").ToLocalChecked(), New<v8::String>((*events)[i]->fromFile).ToLocalChecked());
8475
}
8576

8677
eventArray->Set(i, jsEvent);
@@ -90,14 +81,7 @@ void NSFW::fireEventCallback(uv_async_t *handle) {
9081
eventArray
9182
};
9283

93-
baton->nsfw->mEventCallback->Call(1, argv);
94-
95-
uv_thread_t cleanup;
96-
uv_thread_create(&cleanup, NSFW::cleanupEventCallback, baton);
97-
98-
#if defined(__APPLE_CC__) || defined(__linux__) || defined(__FreeBSD__)
99-
pthread_detach(cleanup);
100-
#endif
84+
nsfw->mEventCallback->Call(1, argv);
10185
}
10286

10387
void NSFW::pollForEvents(void *arg) {
@@ -116,18 +100,14 @@ void NSFW::pollForEvents(void *arg) {
116100
uv_mutex_unlock(&nsfw->mInterfaceLock);
117101
break;
118102
}
119-
auto events = nsfw->mInterface->getEvents();
120-
if (events == NULL) {
103+
104+
if (nsfw->mQueue->count() == 0) {
121105
uv_mutex_unlock(&nsfw->mInterfaceLock);
122106
sleep_for_ms(50);
123107
continue;
124108
}
125109

126-
EventBaton *baton = new EventBaton;
127-
baton->nsfw = nsfw;
128-
baton->events = std::move(events);
129-
130-
nsfw->mEventCallbackAsync.data = (void *)baton;
110+
nsfw->mEventCallbackAsync.data = (void *)nsfw;
131111
uv_async_send(&nsfw->mEventCallbackAsync);
132112

133113
uv_mutex_unlock(&nsfw->mInterfaceLock);
@@ -230,7 +210,8 @@ void NSFW::StartWorker::Execute() {
230210
return;
231211
}
232212

233-
mNSFW->mInterface = new NativeInterface(mNSFW->mPath);
213+
mNSFW->mQueue->clear();
214+
mNSFW->mInterface = new NativeInterface(mNSFW->mPath, mNSFW->mQueue);
234215
if (mNSFW->mInterface->isWatching()) {
235216
mNSFW->mRunning = true;
236217
uv_thread_create(&mNSFW->mPollThread, NSFW::pollForEvents, mNSFW);
@@ -306,6 +287,7 @@ void NSFW::StopWorker::Execute() {
306287
uv_mutex_lock(&mNSFW->mInterfaceLock);
307288
delete mNSFW->mInterface;
308289
mNSFW->mInterface = NULL;
290+
mNSFW->mQueue->clear();
309291

310292
uv_mutex_unlock(&mNSFW->mInterfaceLock);
311293
}

‎src/NativeInterface.cpp

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
#include "../includes/NativeInterface.h"
22

3-
NativeInterface::NativeInterface(const std::string &path) {
4-
mQueue = std::make_shared<EventQueue>();
5-
mNativeInterface.reset(new NativeImplementation(mQueue, path));
3+
NativeInterface::NativeInterface(const std::string &path, std::shared_ptr<EventQueue> queue) {
4+
mNativeInterface.reset(new NativeImplementation(queue, path));
65
}
76

87
NativeInterface::~NativeInterface() {
@@ -13,10 +12,6 @@ std::string NativeInterface::getError() {
1312
return mNativeInterface->getError();
1413
}
1514

16-
std::unique_ptr<std::vector<std::unique_ptr<Event>>> NativeInterface::getEvents() {
17-
return mQueue->dequeueAll();
18-
}
19-
2015
bool NativeInterface::hasErrored() {
2116
return mNativeInterface->hasErrored();
2217
}

0 commit comments

Comments
 (0)