Skip to content

Commit fb0675f

Browse files
ianhattendorfianhattendorf
authored
Merge pull request #1856 from AlexaXs/fix/memory-leak-when-closing-context
Fix memory leak on context shutdown
2 parents 57895a0 + dda1e2b commit fb0675f

12 files changed

Lines changed: 501 additions & 37 deletions

File tree

‎generate/templates/manual/include/context.h‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,23 +8,10 @@
88
#include <uv.h>
99
#include <v8.h>
1010

11-
/*
12-
* Determine if node module is compiled under a supported node release.
13-
* Currently 12 - 15 (ignoring pre-releases). Will need to be updated
14-
* for new major versions.
15-
*
16-
* See: https://github.com/nodejs/node/issues/36349
17-
* and: https://github.com/nodejs/node/blob/master/doc/abi_version_registry.json
18-
*/
19-
#define IS_CONTEXT_AWARE_NODE_MODULE_VERSION \
20-
(NODE_MODULE_VERSION == 72 \
21-
|| NODE_MODULE_VERSION == 79 \
22-
|| NODE_MODULE_VERSION == 83 \
23-
|| NODE_MODULE_VERSION == 88)
24-
2511
#include "async_worker.h"
2612
#include "cleanup_handle.h"
2713
#include "thread_pool.h"
14+
#include "tracker_wrap.h"
2815

2916
namespace nodegit {
3017
class AsyncContextCleanupHandle;
@@ -54,6 +41,14 @@ namespace nodegit {
5441

5542
void ShutdownThreadPool(std::unique_ptr<AsyncContextCleanupHandle> cleanupHandle);
5643

44+
inline void LinkTrackerList(nodegit::TrackerWrap::TrackerList *list) {
45+
list->Link(&trackerList);
46+
}
47+
48+
inline int TrackerListSize() {
49+
return nodegit::TrackerWrap::SizeFromList(&trackerList);
50+
}
51+
5752
private:
5853
v8::Isolate *isolate;
5954

@@ -67,6 +62,8 @@ namespace nodegit {
6762

6863
std::map<std::string, std::shared_ptr<CleanupHandle>> cleanupHandles;
6964

65+
nodegit::TrackerWrap::TrackerList trackerList;
66+
7067
static std::map<v8::Isolate *, Context *> contexts;
7168
};
7269

‎generate/templates/manual/include/nodegit_wrapper.h‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
#ifndef NODEGIT_WRAPPER_H
22
#define NODEGIT_WRAPPER_H
33

4-
#include <nan.h>
54
#include <algorithm>
65
#include <unordered_map>
76

7+
#include "tracker_wrap.h"
88
#include "cleanup_handle.h"
99

1010
// the Traits template parameter supplies:
@@ -16,13 +16,16 @@
1616
//
1717
// static const bool isFreeable
1818
// static void free(cType *raw) - frees the object using freeFunctionName
19+
//
20+
// nodegit::TrackerWrap allows for cheap tracking of new objects, avoiding searchs
21+
// in a container to remove the tracking of a specific object.
1922

2023
namespace nodegit {
2124
class Context;
2225
}
2326

2427
template<typename Traits>
25-
class NodeGitWrapper : public Nan::ObjectWrap {
28+
class NodeGitWrapper : public nodegit::TrackerWrap {
2629
public:
2730
// replicate Traits typedefs for ease of use
2831
typedef typename Traits::cType cType;
@@ -37,7 +40,7 @@ class NodeGitWrapper : public Nan::ObjectWrap {
3740
// a separate issue.
3841
bool selfFreeing;
3942

40-
const nodegit::Context *nodegitContext = nullptr;
43+
nodegit::Context *nodegitContext = nullptr;
4144

4245
protected:
4346
cType *raw;
@@ -68,6 +71,8 @@ class NodeGitWrapper : public Nan::ObjectWrap {
6871
static NAN_METHOD(GetSelfFreeingInstanceCount);
6972
static NAN_METHOD(GetNonSelfFreeingConstructedCount);
7073

74+
void SetNativeOwners(v8::Local<v8::Object> owners);
75+
7176
public:
7277
static v8::Local<v8::Value> New(const cType *raw, bool selfFreeing, v8::Local<v8::Object> owner = v8::Local<v8::Object>());
7378

‎generate/templates/manual/include/tracker_wrap.h‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
#ifndef TRACKERWRAP_H
2+
#define TRACKERWRAP_H
3+
4+
#include <nan.h>
5+
#include <memory>
6+
#include <vector>
7+
8+
namespace nodegit {
9+
// Base class used to track wrapped objects, so that we can
10+
// free the objects that were not freed at the time of context
11+
// closing (because their WeakCallback didn't trigger. See
12+
// https://github.com/nodejs/help/issues/3297).
13+
// Implementation based on node.js's class RefTracker (napi).
14+
class TrackerWrap : public Nan::ObjectWrap {
15+
public:
16+
TrackerWrap() = default;
17+
virtual ~TrackerWrap() = default;
18+
TrackerWrap(const TrackerWrap &other) = delete;
19+
TrackerWrap(TrackerWrap &&other) = delete;
20+
TrackerWrap& operator=(const TrackerWrap &other) = delete;
21+
TrackerWrap& operator=(TrackerWrap &&other) = delete;
22+
23+
// aliases:
24+
// 'TrackerList': used in functionality related to a list.
25+
// 'TrackerWrap' used in functionality not related to a list.
26+
using TrackerList = TrackerWrap;
27+
28+
// Links 'this' right after 'listStart'
29+
inline void Link(TrackerList* listStart) {
30+
m_prev = listStart;
31+
m_next = listStart->m_next;
32+
if (m_next != nullptr) {
33+
m_next->m_prev = this;
34+
}
35+
listStart->m_next = this;
36+
}
37+
38+
// Unlinks itself from the list it's linked to
39+
inline TrackerWrap* Unlink() {
40+
if (m_prev != nullptr) {
41+
m_prev->m_next = m_next;
42+
}
43+
if (m_next != nullptr) {
44+
m_next->m_prev = m_prev;
45+
}
46+
m_prev = nullptr;
47+
m_next = nullptr;
48+
return this;
49+
}
50+
51+
inline void SetTrackerWrapOwners(std::unique_ptr< std::vector<TrackerWrap*> > &&owners) {
52+
m_owners = std::move(owners);
53+
}
54+
55+
inline const std::vector<TrackerWrap*>* GetTrackerWrapOwners() const {
56+
return m_owners.get();
57+
}
58+
59+
// Unlinks and returns the first item of 'listStart'
60+
static TrackerWrap* UnlinkFirst(TrackerList *listStart);
61+
62+
// Returns number of items following 'listStart'
63+
static int SizeFromList(TrackerList *listStart);
64+
65+
// Deletes items following 'listStart', but not 'listStart' itself
66+
static void DeleteFromList(TrackerList *listStart);
67+
68+
private:
69+
TrackerList* m_next {};
70+
TrackerList* m_prev {};
71+
// m_owners will store pointers to native objects
72+
std::unique_ptr< std::vector<TrackerWrap*> > m_owners {};
73+
};
74+
}
75+
76+
#endif

‎generate/templates/manual/revwalk/file_history_walk.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class FileHistoryEvent {
3535

3636
v8::Local<v8::Value> toJavascript() {
3737
v8::Local<v8::Object> historyEntry = Nan::New<v8::Object>();
38-
v8::Local<v8::Array> owners = Nan::New<Array>(1);
38+
v8::Local<v8::Array> owners = Nan::New<Array>(0);
3939
Nan::Set(
4040
owners,
4141
Nan::New<v8::Number>(owners->Length()),

‎generate/templates/manual/src/context.cc‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,7 @@ namespace nodegit {
55

66
AsyncContextCleanupHandle::AsyncContextCleanupHandle(v8::Isolate *isolate, Context *context)
77
: context(context),
8-
#if IS_CONTEXT_AWARE_NODE_MODULE_VERSION
98
handle(node::AddEnvironmentCleanupHook(isolate, AsyncCleanupContext, this))
10-
#else
11-
handle(nullptr)
12-
#endif
139
{}
1410

1511
AsyncContextCleanupHandle::~AsyncContextCleanupHandle() {
@@ -40,6 +36,7 @@ namespace nodegit {
4036
}
4137

4238
Context::~Context() {
39+
nodegit::TrackerWrap::DeleteFromList(&trackerList);
4340
contexts.erase(isolate);
4441
}
4542

‎generate/templates/manual/src/nodegit_wrapper.cc‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
template<typename Traits>
22
NodeGitWrapper<Traits>::NodeGitWrapper(typename Traits::cType *raw, bool selfFreeing, v8::Local<v8::Object> owner)
33
: nodegitContext(nodegit::Context::GetCurrentContext()) {
4+
nodegitContext->LinkTrackerList(this);
45
if (Traits::isSingleton) {
56
ReferenceCounter::incrementCountForPointer((void *)raw);
67
this->raw = raw;
@@ -20,6 +21,7 @@ NodeGitWrapper<Traits>::NodeGitWrapper(typename Traits::cType *raw, bool selfFre
2021
Traits::duplicate(&this->raw, raw);
2122
selfFreeing = true;
2223
} else {
24+
SetNativeOwners(owner);
2325
this->owner.Reset(owner);
2426
this->raw = raw;
2527
}
@@ -45,11 +47,15 @@ NodeGitWrapper<Traits>::NodeGitWrapper(const char *error)
4547

4648
template<typename Traits>
4749
NodeGitWrapper<Traits>::~NodeGitWrapper() {
50+
Unlink();
4851
if (Traits::isFreeable && selfFreeing) {
4952
Traits::free(raw);
5053
SelfFreeingInstanceCount--;
5154
raw = NULL;
5255
}
56+
else if (!selfFreeing) {
57+
--NonSelfFreeingConstructedCount;
58+
}
5359
}
5460

5561
template<typename Traits>
@@ -77,6 +83,33 @@ NAN_METHOD(NodeGitWrapper<Traits>::JSNewFunction) {
7783
info.GetReturnValue().Set(info.This());
7884
}
7985

86+
template<typename Traits>
87+
void NodeGitWrapper<Traits>::SetNativeOwners(v8::Local<v8::Object> owners) {
88+
assert(owners->IsArray() || owners->IsObject());
89+
Nan::HandleScope scope;
90+
std::unique_ptr< std::vector<nodegit::TrackerWrap*> > trackerOwners =
91+
std::make_unique< std::vector<nodegit::TrackerWrap*> >();
92+
93+
if (owners->IsArray()) {
94+
v8::Local<v8::Context> context = Nan::GetCurrentContext();
95+
const v8::Local<v8::Array> ownersArray = owners.As<v8::Array>();
96+
const uint32_t numOwners = ownersArray->Length();
97+
98+
for (uint32_t i = 0; i < numOwners; ++i) {
99+
v8::Local<v8::Value> value = ownersArray->Get(context, i).ToLocalChecked();
100+
const v8::Local<v8::Object> object = value.As<v8::Object>();
101+
Nan::ObjectWrap *objectWrap = Nan::ObjectWrap::Unwrap<Nan::ObjectWrap>(object);
102+
trackerOwners->push_back(static_cast<nodegit::TrackerWrap*>(objectWrap));
103+
}
104+
}
105+
else if (owners->IsObject()) {
106+
Nan::ObjectWrap *objectWrap = Nan::ObjectWrap::Unwrap<Nan::ObjectWrap>(owners);
107+
trackerOwners->push_back(static_cast<nodegit::TrackerWrap*>(objectWrap));
108+
}
109+
110+
SetTrackerWrapOwners(std::move(trackerOwners));
111+
}
112+
80113
template<typename Traits>
81114
v8::Local<v8::Value> NodeGitWrapper<Traits>::New(const typename Traits::cType *raw, bool selfFreeing, v8::Local<v8::Object> owner) {
82115
Nan::EscapableHandleScope scope;

‎generate/templates/manual/src/thread_pool.cc‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -721,19 +721,10 @@ namespace nodegit {
721721
asyncCallbackData->cleanupHandle.swap(cleanupHandle);
722722
asyncCallbackData->pool = nullptr;
723723

724-
#if IS_CONTEXT_AWARE_NODE_MODULE_VERSION
725724
uv_close(reinterpret_cast<uv_handle_t *>(&jsThreadCallbackAsync), [](uv_handle_t *handle) {
726-
auto closeAsyncCallbackData = static_cast<AsyncCallbackData *>(handle->data);
727-
delete closeAsyncCallbackData;
725+
auto asyncCallbackData = static_cast<AsyncCallbackData *>(handle->data);
726+
delete asyncCallbackData;
728727
});
729-
#else
730-
// NOTE We are deliberately leaking this pointer because `async` cleanup in
731-
// node has not completely landed yet. Trying to cleanup this pointer
732-
// is probably not worth the fight as it's very little memory lost per context
733-
// When all LTS versions of node and Electron support async cleanup, we should
734-
// be heading back to cleanup this
735-
uv_close(reinterpret_cast<uv_handle_t *>(&jsThreadCallbackAsync), nullptr);
736-
#endif
737728
}
738729

739730
ThreadPool::ThreadPool(int numberOfThreads, uv_loop_t *loop, nodegit::Context *context)

0 commit comments

Comments
 (0)