From 1abdd394fcf29b2520f0b8f6907b7d7ca614bea3 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Tue, 22 Mar 2016 14:42:33 -0700 Subject: [PATCH 01/12] Add descriptor support for selfFreeing --- generate/scripts/helpers.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/generate/scripts/helpers.js b/generate/scripts/helpers.js index 5383bff58..e9b3268c2 100644 --- a/generate/scripts/helpers.js +++ b/generate/scripts/helpers.js @@ -143,6 +143,7 @@ var Helpers = { } }, + // returns the libgittype found in types decorateLibgitType: function(type, types, enums) { var normalizedType = Helpers.normalizeCtype(type.cType); var libgitType = Helpers.getLibgitType(normalizedType, types); @@ -164,6 +165,8 @@ var Helpers = { // we don't want to overwrite the c type of the passed in type _.merge(type, descriptor.types[normalizedType.replace("git_", "")] || {}, { cType: type.cType }); } + + return libgitType; }, decoratePrimaryType: function(typeDef, enums) { @@ -176,6 +179,7 @@ var Helpers = { typeDef.filename = typeDef.typeName; typeDef.isLibgitType = true; typeDef.dependencies = []; + typeDef.selfFreeing = Boolean(typeDefOverrides.selfFreeing); typeDef.fields = typeDef.fields || []; typeDef.fields.forEach(function (field, index, allFields) { @@ -241,7 +245,7 @@ var Helpers = { arg.cppClassName = Helpers.cTypeToCppName(arg.cType); arg.jsClassName = utils.titleCase(Helpers.cTypeToJsName(arg.cType)); - Helpers.decorateLibgitType(arg, libgit2.types, enums); + var libgitType = Helpers.decorateLibgitType(arg, libgit2.types, enums); // Some arguments can be callbacks if (Helpers.isCallbackFunction(type)) { @@ -271,6 +275,10 @@ var Helpers = { _.every(allArgs, function(_arg) { return !_arg.isSelf; }); } + if (arg.isReturn && libgitType) { + arg.selfFreeing = libgitType.selfFreeing; + } + if (arg.isReturn && fnDef.return && fnDef.return.type === "int") { fnDef.return.isErrorCode = true; fnDef.isAsync = true; From 91d90ed28bee0a8902065c02e0ae854cf32a39a9 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Thu, 17 Mar 2016 09:55:46 -0700 Subject: [PATCH 02/12] Mark commit wrappers as self-freeing Affects commit objects returned by: git_commit_lookup git_commit_lookup_prefix git_commit_nth_gen_ancestor git_commit_parent --- generate/input/descriptor.json | 1 + 1 file changed, 1 insertion(+) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index f51d219ba..2ce8b27bd 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -354,6 +354,7 @@ } }, "commit": { + "selfFreeing": true, "functions": { "git_commit_amend": { "args": { From bc65733f832ed3ff69bfb3d9e79fe00e2892bc21 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Thu, 17 Mar 2016 10:01:02 -0700 Subject: [PATCH 03/12] Mark tree wrappers as self-freeing Affects tree objects returned by: git_tree_lookup git_tree_lookup_prefix --- generate/input/descriptor.json | 1 + 1 file changed, 1 insertion(+) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index 2ce8b27bd..27c3dfcb6 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -2214,6 +2214,7 @@ ] }, "tree": { + "selfFreeing": true, "functions": { "git_tree_entry_free": { "ignore": true From 5a63abdf303142af8a30921d2cfc5a192a626a83 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Thu, 17 Mar 2016 10:09:35 -0700 Subject: [PATCH 04/12] Mark tag wrappers as self-freeing Affects tag objects returned by: git_tag_lookup git_tag_lookup_prefix --- generate/input/descriptor.json | 1 + 1 file changed, 1 insertion(+) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index 27c3dfcb6..e79122646 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -2114,6 +2114,7 @@ } }, "tag": { + "selfFreeing": true, "functions": { "git_tag_foreach": { "ignore": true From f70865825726017b21dfa9f7f595eb7a0475f2e1 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Thu, 17 Mar 2016 10:13:46 -0700 Subject: [PATCH 05/12] Mark blob wrappers as self-freeing Affects blob objects returned by: git_blob_lookup git_blob_lookup_prefix --- generate/input/descriptor.json | 1 + 1 file changed, 1 insertion(+) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index e79122646..ef48e0e7c 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -121,6 +121,7 @@ } }, "blob": { + "selfFreeing": true, "functions": { "git_blob_create_frombuffer": { "isAsync": false, From acd9bedf8c8b74e3bec3963e1832476ecd483fe0 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Tue, 22 Mar 2016 15:34:38 -0700 Subject: [PATCH 06/12] Add self-freeing diagnostic instance counting and leak test --- generate/templates/templates/class_content.cc | 23 +++++++++ generate/templates/templates/class_header.h | 6 +++ test/tests/commit.js | 48 +++++++++++++++++++ 3 files changed, 77 insertions(+) diff --git a/generate/templates/templates/class_content.cc b/generate/templates/templates/class_content.cc index 34d7a0126..92ce14f38 100644 --- a/generate/templates/templates/class_content.cc +++ b/generate/templates/templates/class_content.cc @@ -27,12 +27,21 @@ using namespace node; {{ cppClassName }}::{{ cppClassName }}({{ cType }} *raw, bool selfFreeing) { this->raw = raw; this->selfFreeing = selfFreeing; + + if (selfFreeing) { + SelfFreeingInstanceCount++; + } else { + NonSelfFreeingConstructedCount++; + } + } {{ cppClassName }}::~{{ cppClassName }}() { {% if freeFunctionName %} if (this->selfFreeing) { {{ freeFunctionName }}(this->raw); + SelfFreeingInstanceCount--; + this->raw = NULL; } {% endif %} @@ -77,6 +86,9 @@ using namespace node; {% endif %} {% endeach %} + Nan::SetMethod(tpl, "getSelfFreeingInstanceCount", GetSelfFreeingInstanceCount); + Nan::SetMethod(tpl, "getNonSelfFreeingConstructedCount", GetNonSelfFreeingConstructedCount); + Local _constructor_template = Nan::GetFunction(tpl).ToLocalChecked(); constructor_template.Reset(_constructor_template); Nan::Set(target, Nan::New("{{ jsClassName }}").ToLocalChecked(), _constructor_template); @@ -104,6 +116,14 @@ using namespace node; return scope.Escape(Nan::NewInstance(Nan::New({{ cppClassName }}::constructor_template), 2, argv).ToLocalChecked()); } + NAN_METHOD({{ cppClassName }}::GetSelfFreeingInstanceCount) { + info.GetReturnValue().Set(SelfFreeingInstanceCount); + } + + NAN_METHOD({{ cppClassName }}::GetNonSelfFreeingConstructedCount) { + info.GetReturnValue().Set(NonSelfFreeingConstructedCount); + } + {{ cType }} *{{ cppClassName }}::GetValue() { return this->raw; } @@ -147,3 +167,6 @@ using namespace node; {% if not cTypeIsUndefined %} Nan::Persistent {{ cppClassName }}::constructor_template; {% endif %} + +int {{ cppClassName }}::SelfFreeingInstanceCount; +int {{ cppClassName }}::NonSelfFreeingConstructedCount; diff --git a/generate/templates/templates/class_header.h b/generate/templates/templates/class_header.h index 502b67d72..76976fa3b 100644 --- a/generate/templates/templates/class_header.h +++ b/generate/templates/templates/class_header.h @@ -40,6 +40,10 @@ class {{ cppClassName }} : public Nan::ObjectWrap { static Nan::Persistent constructor_template; static void InitializeComponent (Local target); + // diagnostic count of self-freeing object instances + static int SelfFreeingInstanceCount; + // diagnostic count of constructed non-self-freeing object instances + static int NonSelfFreeingConstructedCount; {%if cType%} {{ cType }} *GetValue(); @@ -96,6 +100,8 @@ class {{ cppClassName }} : public Nan::ObjectWrap { {% endeach %} static NAN_METHOD(JSNewFunction); + static NAN_METHOD(GetSelfFreeingInstanceCount); + static NAN_METHOD(GetNonSelfFreeingConstructedCount); {%each fields as field%} {%if not field.ignore%} diff --git a/test/tests/commit.js b/test/tests/commit.js index 92f122abd..40f617edc 100644 --- a/test/tests/commit.js +++ b/test/tests/commit.js @@ -9,6 +9,25 @@ var exec = promisify(function(command, opts, callback) { return require("child_process").exec(command, opts, callback); }); +// aggressively collects garbage until we fail to improve terminatingIterations +// times. +function garbageCollect() { + var terminatingIterations = 3; + var usedBeforeGC = Number.MAX_VALUE; + var nondecreasingIterations = 0; + for ( ; ; ) { + global.gc(); + var usedAfterGC = process.memoryUsage().heapUsed; + if (usedAfterGC >= usedBeforeGC) { + nondecreasingIterations++; + if (nondecreasingIterations >= terminatingIterations) { + break; + } + } + usedBeforeGC = usedAfterGC; + } +} + describe("Commit", function() { var NodeGit = require("../../"); var Repository = NodeGit.Repository; @@ -624,4 +643,33 @@ describe("Commit", function() { assert.equal(this.committer.email(), "mike@panmedia.co.nz"); }); }); + + it("does not leak", function() { + var test = this; + + garbageCollect(); + var Commit = NodeGit.Commit; + var startSelfFreeingCount = Commit.getSelfFreeingInstanceCount(); + var startNonSelfFreeingCount = Commit.getNonSelfFreeingConstructedCount(); + + var resolve; + var promise = new Promise(function(_resolve) { resolve = _resolve; }); + + NodeGit.Commit.lookup(test.repository, oid) + .then(function() { + // get out of this promise chain to help GC get rid of the commit + setTimeout(resolve, 0); + }); + + return promise + .then(function() { + garbageCollect(); + var endSelfFreeingCount = Commit.getSelfFreeingInstanceCount(); + var endNonSelfFreeingCount = Commit.getNonSelfFreeingConstructedCount(); + // any new self-freeing commits should have been freed + assert.equal(startSelfFreeingCount, endSelfFreeingCount); + // no new non-self-freeing commits should have been constructed + assert.equal(startNonSelfFreeingCount, endNonSelfFreeingCount); + }); + }); }); From 93fd4f20f0d32eda4ec1d66fbdf799dc28506010 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Fri, 18 Mar 2016 10:36:51 -0700 Subject: [PATCH 07/12] Passing correct value for selfFreeing It's possible that we will eventually get rid of the selfFreeing parameter, and just go by the type descriptor, but for now we are more comfortable with a smaller change. --- generate/templates/partials/convert_to_v8.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generate/templates/partials/convert_to_v8.cc b/generate/templates/partials/convert_to_v8.cc index 100885260..9bc61b880 100644 --- a/generate/templates/partials/convert_to_v8.cc +++ b/generate/templates/partials/convert_to_v8.cc @@ -61,7 +61,7 @@ {% if cppClassName == 'Wrapper' %} to = {{ cppClassName }}::New({{= parsedName =}}); {% else %} - to = {{ cppClassName }}::New({{= parsedName =}}, false); + to = {{ cppClassName }}::New({{= parsedName =}}, {% if selfFreeing %}true{% else %}false{% endif %}); {% endif %} } else { From edbb2c8b8d92c953720a425d3e2e2ebff8169974 Mon Sep 17 00:00:00 2001 From: Tyler Wanek Date: Tue, 22 Mar 2016 13:33:24 -0700 Subject: [PATCH 08/12] Force duplication of libgit objects when it is convenient the structs git_tree_entry, git_oid, and git_signature all represent structs which are owned by some other object. In order to simplify our memory model, we should always duplicate these 3 structs when we pull them out of libgit2. This detaches these types of structs from their owning object, such that when we delete a commit, none of the oids or signatures that we retrieved from that commit would cause bad memory access. --- generate/input/descriptor.json | 13 ++++++++++--- generate/scripts/helpers.js | 4 ++++ generate/templates/templates/class_content.cc | 14 +++++++++++++- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index ef48e0e7c..ef7c7e676 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -1275,6 +1275,9 @@ "ignore": true }, "oid": { + "dupFunction": "git_oid_cpy", + "freeFunctionName": "free", + "selfDuplicating": true, "shouldAlloc": true, "functions": { "git_oid_cpy": { @@ -1879,6 +1882,8 @@ } }, "signature": { + "dupFunction": "git_signature_dup", + "selfDuplicating": true, "functions": { "git_signature_default": { "isAsync": false @@ -2218,9 +2223,6 @@ "tree": { "selfFreeing": true, "functions": { - "git_tree_entry_free": { - "ignore": true - }, "git_tree_entry_byindex": { "jsFunctionName": "_entryByIndex" }, @@ -2253,6 +2255,11 @@ } } }, + "tree_entry": { + "dupFunction": "git_tree_entry_dup", + "freeFunctionName": "git_tree_entry_free", + "selfDuplicating": true + }, "writestream": { "cType": "git_writestream", "needsForwardDeclaration": false diff --git a/generate/scripts/helpers.js b/generate/scripts/helpers.js index e9b3268c2..6e1f33613 100644 --- a/generate/scripts/helpers.js +++ b/generate/scripts/helpers.js @@ -181,6 +181,10 @@ var Helpers = { typeDef.dependencies = []; typeDef.selfFreeing = Boolean(typeDefOverrides.selfFreeing); + if (typeDefOverrides.freeFunctionName) { + typeDef.freeFunctionName = typeDefOverrides.freeFunctionName; + } + typeDef.fields = typeDef.fields || []; typeDef.fields.forEach(function (field, index, allFields) { var fieldOverrides = typeDefOverrides.fields || {}; diff --git a/generate/templates/templates/class_content.cc b/generate/templates/templates/class_content.cc index 92ce14f38..334fa9fbb 100644 --- a/generate/templates/templates/class_content.cc +++ b/generate/templates/templates/class_content.cc @@ -25,8 +25,17 @@ using namespace node; {% if cType %} {{ cppClassName }}::{{ cppClassName }}({{ cType }} *raw, bool selfFreeing) { + {% if selfDuplicating %} + {% if shouldAlloc %} + this->raw = ({{ cType }} *)malloc(sizeof({{ cType }})); + {{ dupFunction }}(this->raw, raw); + {% else %} + {{ dupFunction }}(&this->raw, raw); + {% endif %} + {% else %} this->raw = raw; this->selfFreeing = selfFreeing; + {%endif%} if (selfFreeing) { SelfFreeingInstanceCount++; @@ -37,7 +46,10 @@ using namespace node; } {{ cppClassName }}::~{{ cppClassName }}() { - {% if freeFunctionName %} + {% if selfDuplicating %} + {{ freeFunctionName }}(this->raw); + this->raw = NULL; + {% elsif freeFunctionName %} if (this->selfFreeing) { {{ freeFunctionName }}(this->raw); SelfFreeingInstanceCount--; From 2c95f8f7b992e420217c1805516ca1788b20dafc Mon Sep 17 00:00:00 2001 From: John Haley Date: Wed, 23 Mar 2016 08:26:11 -0700 Subject: [PATCH 09/12] Add `toBool` Combyne filter --- generate/scripts/generateNativeCode.js | 1 + generate/templates/filters/to_bool.js | 3 +++ 2 files changed, 4 insertions(+) create mode 100644 generate/templates/filters/to_bool.js diff --git a/generate/scripts/generateNativeCode.js b/generate/scripts/generateNativeCode.js index 93867b49c..4c43e0b04 100644 --- a/generate/scripts/generateNativeCode.js +++ b/generate/scripts/generateNativeCode.js @@ -70,6 +70,7 @@ module.exports = function generateNativeCode() { returnsCount: require("../templates/filters/returns_count"), returnsInfo: require("../templates/filters/returns_info"), titleCase: require("../templates/filters/title_case"), + toBool: require('../templates/filters/to_bool'), unPointer: require("../templates/filters/un_pointer"), upper: require("../templates/filters/upper") }; diff --git a/generate/templates/filters/to_bool.js b/generate/templates/filters/to_bool.js new file mode 100644 index 000000000..dd6063f28 --- /dev/null +++ b/generate/templates/filters/to_bool.js @@ -0,0 +1,3 @@ +module.exports = function(value) { + return !!value; +}; From 47903604e7dbaed519710eda79c57b8cd7f97b2a Mon Sep 17 00:00:00 2001 From: John Haley Date: Wed, 23 Mar 2016 08:26:50 -0700 Subject: [PATCH 10/12] Use `toBool` instead of `if/else` for `selfFreeing` --- generate/templates/partials/convert_to_v8.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generate/templates/partials/convert_to_v8.cc b/generate/templates/partials/convert_to_v8.cc index 9bc61b880..b344ecd26 100644 --- a/generate/templates/partials/convert_to_v8.cc +++ b/generate/templates/partials/convert_to_v8.cc @@ -61,7 +61,7 @@ {% if cppClassName == 'Wrapper' %} to = {{ cppClassName }}::New({{= parsedName =}}); {% else %} - to = {{ cppClassName }}::New({{= parsedName =}}, {% if selfFreeing %}true{% else %}false{% endif %}); + to = {{ cppClassName }}::New({{= parsedName =}}, {{ selfFreeing|toBool }}); {% endif %} } else { From 11a17cba7b5795f68659623510a8ac250dbbe247 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 23 Mar 2016 16:13:25 -0700 Subject: [PATCH 11/12] Add shouldDuplicate as arg-specific behavior --- generate/input/descriptor.json | 70 +++++++++++++++++-- generate/templates/partials/convert_to_v8.cc | 2 +- generate/templates/templates/class_content.cc | 28 ++++---- generate/templates/templates/class_header.h | 4 +- 4 files changed, 84 insertions(+), 20 deletions(-) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index ef7c7e676..29f9e2ce4 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -141,6 +141,11 @@ "git_blob_filtered_content": { "ignore": true }, + "git_blob_id": { + "return": { + "shouldDuplicate": true + } + }, "git_blob_rawcontent": { "return": { "cppClassName": "Wrapper", @@ -382,6 +387,16 @@ } } }, + "git_commit_author": { + "return": { + "shouldDuplicate": true + } + }, + "git_commit_committer": { + "return": { + "shouldDuplicate": true + } + }, "git_commit_create": { "args": { "id": { @@ -406,6 +421,21 @@ }, "git_commit_create_from_ids": { "ignore": true + }, + "git_commit_id": { + "return": { + "shouldDuplicate": true + } + }, + "git_commit_parent_id": { + "return": { + "shouldDuplicate": true + } + }, + "git_commit_tree_id": { + "return": { + "shouldDuplicate": true + } } } }, @@ -1277,7 +1307,6 @@ "oid": { "dupFunction": "git_oid_cpy", "freeFunctionName": "free", - "selfDuplicating": true, "shouldAlloc": true, "functions": { "git_oid_cpy": { @@ -1883,7 +1912,6 @@ }, "signature": { "dupFunction": "git_signature_dup", - "selfDuplicating": true, "functions": { "git_signature_default": { "isAsync": false @@ -2139,6 +2167,11 @@ "git_tag_create_frombuffer": { "ignore": true }, + "git_tag_id": { + "return": { + "shouldDuplicate": true + } + }, "git_tag_create_lightweight": { "args": { "oid": { @@ -2174,6 +2207,11 @@ }, "isAsync": true }, + "git_tag_tagger": { + "return": { + "shouldDuplicate": true + } + }, "git_tag_target": { "args": { "target_out": { @@ -2181,6 +2219,11 @@ } } }, + "git_tag_target_id": { + "return": { + "shouldDuplicate": true + } + }, "git_tag_delete": { "return": { "isErrorCode": true @@ -2223,12 +2266,32 @@ "tree": { "selfFreeing": true, "functions": { + "git_tree_entry_byid": { + "return": { + "shouldDuplicate": true + } + }, "git_tree_entry_byindex": { "jsFunctionName": "_entryByIndex" }, + "git_tree_entry_byname": { + "return": { + "shouldDuplicate": true + } + }, + "git_tree_entry_id": { + "return": { + "shouldDuplicate": true + } + }, "git_tree_entrycount": { "jsFunctionName": "entryCount" }, + "git_tree_id": { + "return": { + "shouldDuplicate": true + } + }, "git_tree_walk": { "ignore": true } @@ -2257,8 +2320,7 @@ }, "tree_entry": { "dupFunction": "git_tree_entry_dup", - "freeFunctionName": "git_tree_entry_free", - "selfDuplicating": true + "freeFunctionName": "git_tree_entry_free" }, "writestream": { "cType": "git_writestream", diff --git a/generate/templates/partials/convert_to_v8.cc b/generate/templates/partials/convert_to_v8.cc index b344ecd26..4c669ac76 100644 --- a/generate/templates/partials/convert_to_v8.cc +++ b/generate/templates/partials/convert_to_v8.cc @@ -61,7 +61,7 @@ {% if cppClassName == 'Wrapper' %} to = {{ cppClassName }}::New({{= parsedName =}}); {% else %} - to = {{ cppClassName }}::New({{= parsedName =}}, {{ selfFreeing|toBool }}); + to = {{ cppClassName }}::New({{= parsedName =}}, {{ selfFreeing|toBool }} {% if shouldDuplicate %}, true{% endif %}); {% endif %} } else { diff --git a/generate/templates/templates/class_content.cc b/generate/templates/templates/class_content.cc index 334fa9fbb..312c2ef28 100644 --- a/generate/templates/templates/class_content.cc +++ b/generate/templates/templates/class_content.cc @@ -24,18 +24,19 @@ using namespace v8; using namespace node; {% if cType %} - {{ cppClassName }}::{{ cppClassName }}({{ cType }} *raw, bool selfFreeing) { - {% if selfDuplicating %} + {{ cppClassName }}::{{ cppClassName }}({{ cType }} *raw, bool selfFreeing, bool shouldDuplicate) { + if (shouldDuplicate) { {% if shouldAlloc %} this->raw = ({{ cType }} *)malloc(sizeof({{ cType }})); {{ dupFunction }}(this->raw, raw); {% else %} {{ dupFunction }}(&this->raw, raw); {% endif %} - {% else %} - this->raw = raw; + selfFreeing = true; + } else { + this->raw = raw; + } this->selfFreeing = selfFreeing; - {%endif%} if (selfFreeing) { SelfFreeingInstanceCount++; @@ -46,10 +47,7 @@ using namespace node; } {{ cppClassName }}::~{{ cppClassName }}() { - {% if selfDuplicating %} - {{ freeFunctionName }}(this->raw); - this->raw = NULL; - {% elsif freeFunctionName %} + {% if freeFunctionName %} if (this->selfFreeing) { {{ freeFunctionName }}(this->raw); SelfFreeingInstanceCount--; @@ -116,16 +114,20 @@ using namespace node; {% endif %} } - {{ cppClassName }}* object = new {{ cppClassName }}(static_cast<{{ cType }} *>(Local::Cast(info[0])->Value()), Nan::To(info[1]).FromJust()); + {{ cppClassName }}* object = new {{ cppClassName }}(static_cast<{{ cType }} *>( + Local::Cast(info[0])->Value()), + Nan::To(info[1]).FromJust(), + info.Length() >= 3 ? Nan::To(info[2]).FromJust() : false + ); object->Wrap(info.This()); info.GetReturnValue().Set(info.This()); } - Local {{ cppClassName }}::New(const {{ cType }} *raw, bool selfFreeing) { + Local {{ cppClassName }}::New(const {{ cType }} *raw, bool selfFreeing, bool shouldDuplicate) { Nan::EscapableHandleScope scope; - Local argv[2] = { Nan::New((void *)raw), Nan::New(selfFreeing) }; - return scope.Escape(Nan::NewInstance(Nan::New({{ cppClassName }}::constructor_template), 2, argv).ToLocalChecked()); + Local argv[3] = { Nan::New((void *)raw), Nan::New(selfFreeing), Nan::New(shouldDuplicate) }; + return scope.Escape(Nan::NewInstance(Nan::New({{ cppClassName }}::constructor_template), 3, argv).ToLocalChecked()); } NAN_METHOD({{ cppClassName }}::GetSelfFreeingInstanceCount) { diff --git a/generate/templates/templates/class_header.h b/generate/templates/templates/class_header.h index 76976fa3b..cc77f52b3 100644 --- a/generate/templates/templates/class_header.h +++ b/generate/templates/templates/class_header.h @@ -49,7 +49,7 @@ class {{ cppClassName }} : public Nan::ObjectWrap { {{ cType }} *GetValue(); void ClearValue(); - static Local New(const {{ cType }} *raw, bool selfFreeing); + static Local New(const {{ cType }} *raw, bool selfFreeing, bool shouldDuplicate = false); {%endif%} bool selfFreeing; @@ -85,7 +85,7 @@ class {{ cppClassName }} : public Nan::ObjectWrap { {%if cType%} - {{ cppClassName }}({{ cType }} *raw, bool selfFreeing); + {{ cppClassName }}({{ cType }} *raw, bool selfFreeing, bool shouldDuplicate = false); ~{{ cppClassName }}(); {%endif%} From 4283436b9312dc44dda26b5ba7707a514d823b5e Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 23 Mar 2016 16:20:25 -0700 Subject: [PATCH 12/12] Add test for shouldDuplicate --- test/tests/commit.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/tests/commit.js b/test/tests/commit.js index 40f617edc..e770a0c1e 100644 --- a/test/tests/commit.js +++ b/test/tests/commit.js @@ -672,4 +672,26 @@ describe("Commit", function() { assert.equal(startNonSelfFreeingCount, endNonSelfFreeingCount); }); }); + + it("duplicates signature", function() { + garbageCollect(); + var Signature = NodeGit.Signature; + var startSelfFreeingCount = Signature.getSelfFreeingInstanceCount(); + var startNonSelfFreeingCount = + Signature.getNonSelfFreeingConstructedCount(); + var signature = this.commit.author(); + + garbageCollect(); + var endSelfFreeingCount = Signature.getSelfFreeingInstanceCount(); + var endNonSelfFreeingCount = Signature.getNonSelfFreeingConstructedCount(); + // we should get one duplicated, self-freeing signature + assert.equal(startSelfFreeingCount + 1, endSelfFreeingCount); + assert.equal(startNonSelfFreeingCount, endNonSelfFreeingCount); + + signature = null; + garbageCollect(); + endSelfFreeingCount = Signature.getSelfFreeingInstanceCount(); + // the self-freeing signature should get freed + assert.equal(startSelfFreeingCount, endSelfFreeingCount); + }); });