Add additional GC behavior that reflects new descriptor format

implausible · implausible · commit 2e5451fcd048 · 2018-10-16T18:20:20.000-07:00
- Implements isSingleton behavior with reference counting
- Implements ownerFn, which ties an arbitrary pointer to a singleton type pointer
- always calls free, even if object is "owned", the ownership link is about keeping internal pointers alive, but it seems that most calls to free are reference counted in libgit2, but the repository has a different relationship than all other types of ref counted frees.
diff --git a/generate/templates/filters/returns_info.js b/generate/templates/filters/returns_info.js
@@ -12,8 +12,12 @@ module.exports = function(fn, argReturnsOnly, isAsync) {
   // any sort of string to argument index
   // in the template.
   var nameToArgIndex = {};
+  var thisArgName = '';
   args.forEach(function (arg, index) {
     nameToArgIndex[arg.name] = index;
+    if (arg.isSelf) {
+      thisArgName = arg.name;
+    }
   });
 
   args.forEach(function (arg) {
@@ -34,9 +38,13 @@ module.exports = function(fn, argReturnsOnly, isAsync) {
     return_info.returnNameOrName = return_info.returnName || return_info.name;
     return_info.jsOrCppClassName = return_info.jsClassName || return_info.cppClassName;
     return_info.isOutParam = true;
-    return_info.hasOwner = !!(return_info.ownedBy || return_info.ownedByThis);
+    return_info.hasOwner = !!(return_info.ownedBy || return_info.ownedByThis || return_info.ownerFn);
     return_info.ownedByIndex = -1;
 
+    if (return_info.ownedByThis) {
+      return_info.ownedBy = thisArgName;
+    }
+
     // Here we convert ownedBy, which is the name of the parameter
     // that owns this result to the argument index.
     // sync functions will need to know this.
@@ -57,7 +65,10 @@ module.exports = function(fn, argReturnsOnly, isAsync) {
     return_info.__proto__ = fn.return;
 
     return_info.isAsync = isAsync;
-    return_info.hasOwner = !!return_info.ownedByThis;
+    return_info.hasOwner = !!(return_info.ownedByThis || return_info.ownerFn);
+    if (return_info.ownedByThis) {
+      return_info.ownedBy = thisArgName;
+    }
     return_info.ownedByIndex = -1;
     return_info.parsedName = return_info.name && isAsync ? "baton->" + return_info.name : "result";
     return_info.isCppClassIntType = ~['Uint32', 'Int32'].indexOf(return_info.cppClassName);
diff --git a/generate/templates/manual/include/nodegit_wrapper.h b/generate/templates/manual/include/nodegit_wrapper.h
@@ -2,6 +2,7 @@
 #define NODEGIT_WRAPPER_H
 
 #include <nan.h>
+#include <unordered_map>
 
 // the Traits template parameter supplies:
 //  typename cppClass - the C++ type of the NodeGit wrapper (e.g. GitRepository)
diff --git a/generate/templates/manual/include/reference_counter.h b/generate/templates/manual/include/reference_counter.h
@@ -0,0 +1,30 @@
+#ifndef REFERENCE_COUNTER_H
+#define REFERENCE_COUNTER_H
+
+#include <unordered_map>
+
+#include "lock_master.h"
+
+// There are certain instances in libgit2 which can be retrieved from multiple sources
+// We need to make sure that we're counting how many times we've seen that pointer
+// so that when we are performing free behavior, we don't free it until it is no longer
+// referenced. The main example of this behavior is the repository instance, where
+// after git_repository_open open (first instance) we can git_commit_lookup, followed by
+// git_commit_owner (second instance).
+//
+// I was hoping that we could construct a Persistent handle, but that would interfere with
+// GC. We want it to attmept to GC, and if this handle exists, the final repo will not
+// free itself :(.
+//
+// Make sure to utilize LockMaster when incrementing or decrementing a reference count.
+class ReferenceCounter {
+public:
+  static void incrementCountForPointer(void *ptr);
+
+  static unsigned long decrementCountForPointer(void *ptr);
+
+private:
+  static std::unordered_map<void *, unsigned long> referenceCountByPointer;
+};
+
+#endif
diff --git a/generate/templates/manual/src/nodegit_wrapper.cc b/generate/templates/manual/src/nodegit_wrapper.cc
@@ -1,17 +1,26 @@
 template<typename Traits>
 NodeGitWrapper<Traits>::NodeGitWrapper(typename Traits::cType *raw, bool selfFreeing, v8::Local<v8::Object> owner) {
-  if (!owner.IsEmpty()) {
-    // if we have an owner, there are two options - either we duplicate the raw object
-    // (so we own the duplicate, and can self-free it)
-    // or we keep a handle on the owner so it doesn't get garbage collected
-    // while this wrapper is accessible
-    if(Traits::isDuplicable) {
+  if (Traits::isSingleton) {
+    ReferenceCounter::incrementCountForPointer((void *)raw);
+    this->raw = raw;
+  } else if (!owner.IsEmpty()) {
+    // if we have an owner, it could mean 2 things:
+    //  1. We are borrowed memory from another struct and should not be freed. We will keep a handle to the owner
+    //     so that the owner isn't gc'd while we are using its memory.
+    //  2. We are borrowed memory from another struct and can be duplicated, so we should duplicate
+    //     and become selfFreeing.
+    //  3. We are cached memory, potentially on the repo or config.
+    //     Even though we have a handle in another objects cache, we are expected to call free,
+    //     otherwise we are leaking memory. Cached objects are reference counted in libgit2, but will be leaked
+    //     even if the cache is cleared if we haven't freed them. We will keep a handle on the owner, even though it
+    //     is probably safe as we're reference counted. This should at worst just ensure that the cache owner is the
+    //     last thing to be freed, and that is more safety than anything else.
+    if (Traits::isDuplicable) {
       Traits::duplicate(&this->raw, raw);
       selfFreeing = true;
     } else {
       this->owner.Reset(owner);
       this->raw = raw;
-      selfFreeing = false;
     }
   } else {
     this->raw = raw;
@@ -34,7 +43,7 @@ NodeGitWrapper<Traits>::NodeGitWrapper(const char *error) {
 
 template<typename Traits>
 NodeGitWrapper<Traits>::~NodeGitWrapper() {
-  if(Traits::isFreeable && selfFreeing) {
+  if (Traits::isFreeable && selfFreeing) {
     Traits::free(raw);
     SelfFreeingInstanceCount--;
     raw = NULL;
diff --git a/generate/templates/manual/src/reference_counter.cc b/generate/templates/manual/src/reference_counter.cc
@@ -0,0 +1,24 @@
+#include "../include/reference_counter.h"
+
+void ReferenceCounter::incrementCountForPointer(void *ptr) {
+  LockMaster(true, &referenceCountByPointer);
+  if (referenceCountByPointer.find(ptr) == referenceCountByPointer.end()) {
+    referenceCountByPointer[ptr] = 1;
+  } else {
+    referenceCountByPointer[ptr] = referenceCountByPointer[ptr] + 1;
+  }
+}
+
+unsigned long ReferenceCounter::decrementCountForPointer(void *ptr) {
+  LockMaster(true, &referenceCountByPointer);
+  unsigned long referenceCount = referenceCountByPointer[ptr];
+  if (referenceCount == 1) {
+    referenceCountByPointer.erase(ptr);
+    return 0;
+  } else {
+    referenceCountByPointer[ptr] = referenceCount - 1;
+    return referenceCountByPointer[ptr];
+  }
+}
+
+std::unordered_map<void *, unsigned long> ReferenceCounter::referenceCountByPointer;
diff --git a/generate/templates/partials/convert_to_v8.cc b/generate/templates/partials/convert_to_v8.cc
@@ -70,10 +70,15 @@
         {{ selfFreeing|toBool }}
         {% if hasOwner %}
           ,
-          {% if ownedByThis %}
-            info.This()
+          {% if ownerFn | toBool %}
+            {{= ownerFn.singletonCppClassName =}}::New(
+              {{= ownerFn.name =}}({{= parsedName =}}),
+              true
+            )->ToObject()
           {% elsif isAsync %}
-            this->GetFromPersistent("{{= ownedBy =}}")
+            this->GetFromPersistent("{{= ownedBy =}}")->ToObject()
+          {% elsif ownedByThis %}
+            info.This()
           {% else %}
             info[{{= ownedByIndex =}}]->ToObject()
           {% endif %}
diff --git a/generate/templates/partials/traits.h b/generate/templates/partials/traits.h
@@ -17,10 +17,17 @@ struct {{ cppClassName }}Traits {
   {% endif %}
   }
 
+  static const bool isSingleton = {{ isSingleton | toBool }};
   static const bool isFreeable = {{ freeFunctionName | toBool}};
   static void free({{ cType }} *raw) {
   {% if freeFunctionName %}
-    ::{{ freeFunctionName }}(raw); // :: to avoid calling this free recursively
+    unsigned long referenceCount = 0;
+    {% if isSingleton %}
+    referenceCount = ReferenceCounter::decrementCountForPointer((void *)raw);
+    {% endif %}
+    if (referenceCount == 0) {
+      ::{{ freeFunctionName }}(raw); // :: to avoid calling this free recursively
+    }
   {% else %}
     Nan::ThrowError("free called on {{ cppClassName }} which cannot be freed");
   {% endif %}
diff --git a/generate/templates/templates/binding.gyp b/generate/templates/templates/binding.gyp
@@ -75,6 +75,7 @@
       "sources": [
         "src/async_baton.cc",
         "src/lock_master.cc",
+        "src/reference_counter.cc",
         "src/nodegit.cc",
         "src/init_ssh2.cc",
         "src/promise_completion.cc",
diff --git a/generate/templates/templates/class_header.h b/generate/templates/templates/class_header.h
@@ -4,10 +4,12 @@
 #include <string>
 #include <queue>
 #include <utility>
+#include <unordered_map>
 
 #include "async_baton.h"
 #include "nodegit_wrapper.h"
 #include "promise_completion.h"
+#include "reference_counter.h"
 
 extern "C" {
 #include <git2.h>
diff --git a/generate/templates/templates/struct_header.h b/generate/templates/templates/struct_header.h
@@ -4,9 +4,11 @@
 #include <string>
 #include <queue>
 #include <utility>
+#include <unordered_map>
 
 #include "async_baton.h"
 #include "callback_wrapper.h"
+#include "reference_counter.h"
 #include "nodegit_wrapper.h"
 
 extern "C" {
