nodegit · ianhattendorf · Oct 27, 2021 · Sep 7, 2021 · Aug 31, 2021 · Sep 8, 2021
diff --git a/generate/templates/manual/include/context.h b/generate/templates/manual/include/context.h
@@ -8,23 +8,10 @@
 #include <uv.h>
 #include <v8.h>
 
-/*
- * Determine if node module is compiled under a supported node release.
- * Currently 12 - 15 (ignoring pre-releases). Will need to be updated
- * for new major versions.
- *
- * See: https://github.com/nodejs/node/issues/36349
- * and: https://github.com/nodejs/node/blob/master/doc/abi_version_registry.json
- */
-#define IS_CONTEXT_AWARE_NODE_MODULE_VERSION \
-  (NODE_MODULE_VERSION == 72 \
-  || NODE_MODULE_VERSION == 79 \
-  || NODE_MODULE_VERSION == 83 \
-  || NODE_MODULE_VERSION == 88)
-
 #include "async_worker.h"
 #include "cleanup_handle.h"
 #include "thread_pool.h"
+#include "tracker_wrap.h"
 
 namespace nodegit {
   class AsyncContextCleanupHandle;
@@ -54,6 +41,14 @@ namespace nodegit {
 
     void ShutdownThreadPool(std::unique_ptr<AsyncContextCleanupHandle> cleanupHandle);
 
+    inline void LinkTrackerList(nodegit::TrackerWrap::TrackerList *list) {
+      list->Link(&trackerList);
+    }
+
+    inline int TrackerListSize() {
+      return nodegit::TrackerWrap::SizeFromList(&trackerList);
+    }
+
   private:
     v8::Isolate *isolate;
 
@@ -67,6 +62,8 @@ namespace nodegit {
 
     std::map<std::string, std::shared_ptr<CleanupHandle>> cleanupHandles;
 
+    nodegit::TrackerWrap::TrackerList trackerList;
+
     static std::map<v8::Isolate *, Context *> contexts;
   };
 

diff --git a/generate/templates/manual/include/nodegit_wrapper.h b/generate/templates/manual/include/nodegit_wrapper.h
@@ -1,10 +1,10 @@
 #ifndef NODEGIT_WRAPPER_H
 #define NODEGIT_WRAPPER_H
 
-#include <nan.h>
 #include <algorithm>
 #include <unordered_map>
 
+#include "tracker_wrap.h"
 #include "cleanup_handle.h"
 
 // the Traits template parameter supplies:
@@ -16,13 +16,16 @@
 //
 //  static const bool isFreeable
 //  static void free(cType *raw) - frees the object using freeFunctionName
+//
+// nodegit::TrackerWrap allows for cheap tracking of new objects, avoiding searchs
+// in a container to remove the tracking of a specific object.
 
 namespace nodegit {
   class Context;
 }
 
 template<typename Traits>
-class NodeGitWrapper : public Nan::ObjectWrap {
+class NodeGitWrapper : public nodegit::TrackerWrap {
 public:
   // replicate Traits typedefs for ease of use
   typedef typename Traits::cType cType;
@@ -37,7 +40,7 @@ class NodeGitWrapper : public Nan::ObjectWrap {
   // a separate issue.
   bool selfFreeing;
 
-  const nodegit::Context *nodegitContext = nullptr;
+  nodegit::Context *nodegitContext = nullptr;
 
 protected:
   cType *raw;
@@ -68,6 +71,8 @@ class NodeGitWrapper : public Nan::ObjectWrap {
   static NAN_METHOD(GetSelfFreeingInstanceCount);
   static NAN_METHOD(GetNonSelfFreeingConstructedCount);
 
+  void SetNativeOwners(v8::Local<v8::Object> owners);
+
 public:
   static v8::Local<v8::Value> New(const cType *raw, bool selfFreeing, v8::Local<v8::Object> owner = v8::Local<v8::Object>());
 

diff --git a/generate/templates/manual/include/tracker_wrap.h b/generate/templates/manual/include/tracker_wrap.h
@@ -0,0 +1,76 @@
+#ifndef TRACKERWRAP_H
+#define TRACKERWRAP_H
+
+#include <nan.h>
+#include <memory>
+#include <vector>
+
+namespace nodegit {
+  // Base class used to track wrapped objects, so that we can
+  // free the objects that were not freed at the time of context
+  // closing (because their WeakCallback didn't trigger. See
+  // https://github.com/nodejs/help/issues/3297).
+  // Implementation based on node.js's class RefTracker (napi).
+  class TrackerWrap : public Nan::ObjectWrap {
+  public:
+    TrackerWrap() = default;
+    virtual ~TrackerWrap() = default;
+    TrackerWrap(const TrackerWrap &other) = delete;
+    TrackerWrap(TrackerWrap &&other) = delete;
+    TrackerWrap& operator=(const TrackerWrap &other) = delete;
+    TrackerWrap& operator=(TrackerWrap &&other) = delete;
+
+    // aliases:
+    // 'TrackerList': used in functionality related to a list.
+    // 'TrackerWrap' used in functionality not related to a list.
+    using TrackerList = TrackerWrap;
+
+    // Links 'this' right after 'listStart'
+    inline void Link(TrackerList* listStart) {
+      m_prev = listStart;
+      m_next = listStart->m_next;
+      if (m_next != nullptr) {
+        m_next->m_prev = this;
+      }
+      listStart->m_next = this;
+    }
+
+    // Unlinks itself from the list it's linked to
+    inline TrackerWrap* Unlink() {
+      if (m_prev != nullptr) {
+        m_prev->m_next = m_next;
+      }
+      if (m_next != nullptr) {
+        m_next->m_prev = m_prev;
+      }
+      m_prev = nullptr;
+      m_next = nullptr;
+      return this;
+    }
+
+    inline void SetTrackerWrapOwners(std::unique_ptr< std::vector<TrackerWrap*> > &&owners) {
+      m_owners = std::move(owners);
+    }
+
+    inline const std::vector<TrackerWrap*>* GetTrackerWrapOwners() const {
+      return m_owners.get();
+    }
+
+    // Unlinks and returns the first item of 'listStart'
+    static TrackerWrap* UnlinkFirst(TrackerList *listStart);
+
+    // Returns number of items following 'listStart'
+    static int SizeFromList(TrackerList *listStart);
+
+    // Deletes items following 'listStart', but not 'listStart' itself
+    static void DeleteFromList(TrackerList *listStart);
+
+  private:
+    TrackerList* m_next {};
+    TrackerList* m_prev {};
+    // m_owners will store pointers to native objects
+    std::unique_ptr< std::vector<TrackerWrap*> > m_owners {};
+  };
+}
+
+#endif
diff --git a/generate/templates/manual/revwalk/file_history_walk.cc b/generate/templates/manual/revwalk/file_history_walk.cc
@@ -35,7 +35,7 @@ class FileHistoryEvent {
 
   v8::Local<v8::Value> toJavascript() {
     v8::Local<v8::Object> historyEntry = Nan::New<v8::Object>();
-    v8::Local<v8::Array> owners = Nan::New<Array>(1);
+    v8::Local<v8::Array> owners = Nan::New<Array>(0);
     Nan::Set(
       owners,
       Nan::New<v8::Number>(owners->Length()),

diff --git a/generate/templates/manual/src/context.cc b/generate/templates/manual/src/context.cc
@@ -5,11 +5,7 @@ namespace nodegit {
 
   AsyncContextCleanupHandle::AsyncContextCleanupHandle(v8::Isolate *isolate, Context *context)
     : context(context),
-#if IS_CONTEXT_AWARE_NODE_MODULE_VERSION
       handle(node::AddEnvironmentCleanupHook(isolate, AsyncCleanupContext, this))
-#else
-      handle(nullptr)
-#endif
   {}
 
   AsyncContextCleanupHandle::~AsyncContextCleanupHandle() {
@@ -40,6 +36,7 @@ namespace nodegit {
   }
 
   Context::~Context() {
+    nodegit::TrackerWrap::DeleteFromList(&trackerList);
     contexts.erase(isolate);
   }
 

diff --git a/generate/templates/manual/src/nodegit_wrapper.cc b/generate/templates/manual/src/nodegit_wrapper.cc
@@ -1,6 +1,7 @@
 template<typename Traits>
 NodeGitWrapper<Traits>::NodeGitWrapper(typename Traits::cType *raw, bool selfFreeing, v8::Local<v8::Object> owner)
   : nodegitContext(nodegit::Context::GetCurrentContext()) {
+  nodegitContext->LinkTrackerList(this);
   if (Traits::isSingleton) {
     ReferenceCounter::incrementCountForPointer((void *)raw);
     this->raw = raw;
@@ -20,6 +21,7 @@ NodeGitWrapper<Traits>::NodeGitWrapper(typename Traits::cType *raw, bool selfFre
       Traits::duplicate(&this->raw, raw);
       selfFreeing = true;
     } else {
+      SetNativeOwners(owner);
       this->owner.Reset(owner);
       this->raw = raw;
     }
@@ -45,11 +47,15 @@ NodeGitWrapper<Traits>::NodeGitWrapper(const char *error)
 
 template<typename Traits>
 NodeGitWrapper<Traits>::~NodeGitWrapper() {
+  Unlink();
   if (Traits::isFreeable && selfFreeing) {
     Traits::free(raw);
     SelfFreeingInstanceCount--;
     raw = NULL;
   }
+  else if (!selfFreeing) {
+    --NonSelfFreeingConstructedCount;
+  }
 }
 
 template<typename Traits>
@@ -77,6 +83,33 @@ NAN_METHOD(NodeGitWrapper<Traits>::JSNewFunction) {
   info.GetReturnValue().Set(info.This());
 }
 
+template<typename Traits>
+void NodeGitWrapper<Traits>::SetNativeOwners(v8::Local<v8::Object> owners) {
+  assert(owners->IsArray() || owners->IsObject());
+  Nan::HandleScope scope;
+  std::unique_ptr< std::vector<nodegit::TrackerWrap*> > trackerOwners = 
+    std::make_unique< std::vector<nodegit::TrackerWrap*> >();
+
+  if (owners->IsArray()) {
+    v8::Local<v8::Context> context = Nan::GetCurrentContext();
+    const v8::Local<v8::Array> ownersArray = owners.As<v8::Array>();
+    const uint32_t numOwners = ownersArray->Length();
+
+    for (uint32_t i = 0; i < numOwners; ++i) {
+      v8::Local<v8::Value> value = ownersArray->Get(context, i).ToLocalChecked();
+      const v8::Local<v8::Object> object = value.As<v8::Object>();
+      Nan::ObjectWrap *objectWrap = Nan::ObjectWrap::Unwrap<Nan::ObjectWrap>(object);
+      trackerOwners->push_back(static_cast<nodegit::TrackerWrap*>(objectWrap));
+    }
+  }
+  else if (owners->IsObject()) {
+    Nan::ObjectWrap *objectWrap = Nan::ObjectWrap::Unwrap<Nan::ObjectWrap>(owners);
+    trackerOwners->push_back(static_cast<nodegit::TrackerWrap*>(objectWrap));
+  }
+
+  SetTrackerWrapOwners(std::move(trackerOwners));
+}
+
 template<typename Traits>
 v8::Local<v8::Value> NodeGitWrapper<Traits>::New(const typename Traits::cType *raw, bool selfFreeing, v8::Local<v8::Object> owner) {
   Nan::EscapableHandleScope scope;

diff --git a/generate/templates/manual/src/thread_pool.cc b/generate/templates/manual/src/thread_pool.cc
@@ -721,19 +721,10 @@ namespace nodegit {
     asyncCallbackData->cleanupHandle.swap(cleanupHandle);
     asyncCallbackData->pool = nullptr;
 
-#if IS_CONTEXT_AWARE_NODE_MODULE_VERSION
     uv_close(reinterpret_cast<uv_handle_t *>(&jsThreadCallbackAsync), [](uv_handle_t *handle) {
-      auto closeAsyncCallbackData = static_cast<AsyncCallbackData *>(handle->data);
-      delete closeAsyncCallbackData;
+      auto asyncCallbackData = static_cast<AsyncCallbackData *>(handle->data);
+      delete asyncCallbackData;
     });
-#else
-    // NOTE We are deliberately leaking this pointer because `async` cleanup in
-    // node has not completely landed yet. Trying to cleanup this pointer
-    // is probably not worth the fight as it's very little memory lost per context
-    // When all LTS versions of node and Electron support async cleanup, we should
-    // be heading back to cleanup this
-    uv_close(reinterpret_cast<uv_handle_t *>(&jsThreadCallbackAsync), nullptr);
-#endif
   }
 
   ThreadPool::ThreadPool(int numberOfThreads, uv_loop_t *loop, nodegit::Context *context)