diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index f51d219ba..29f9e2ce4 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, @@ -140,6 +141,11 @@ "git_blob_filtered_content": { "ignore": true }, + "git_blob_id": { + "return": { + "shouldDuplicate": true + } + }, "git_blob_rawcontent": { "return": { "cppClassName": "Wrapper", @@ -354,6 +360,7 @@ } }, "commit": { + "selfFreeing": true, "functions": { "git_commit_amend": { "args": { @@ -380,6 +387,16 @@ } } }, + "git_commit_author": { + "return": { + "shouldDuplicate": true + } + }, + "git_commit_committer": { + "return": { + "shouldDuplicate": true + } + }, "git_commit_create": { "args": { "id": { @@ -404,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 + } } } }, @@ -1273,6 +1305,8 @@ "ignore": true }, "oid": { + "dupFunction": "git_oid_cpy", + "freeFunctionName": "free", "shouldAlloc": true, "functions": { "git_oid_cpy": { @@ -1877,6 +1911,7 @@ } }, "signature": { + "dupFunction": "git_signature_dup", "functions": { "git_signature_default": { "isAsync": false @@ -2113,6 +2148,7 @@ } }, "tag": { + "selfFreeing": true, "functions": { "git_tag_foreach": { "ignore": true @@ -2131,6 +2167,11 @@ "git_tag_create_frombuffer": { "ignore": true }, + "git_tag_id": { + "return": { + "shouldDuplicate": true + } + }, "git_tag_create_lightweight": { "args": { "oid": { @@ -2166,6 +2207,11 @@ }, "isAsync": true }, + "git_tag_tagger": { + "return": { + "shouldDuplicate": true + } + }, "git_tag_target": { "args": { "target_out": { @@ -2173,6 +2219,11 @@ } } }, + "git_tag_target_id": { + "return": { + "shouldDuplicate": true + } + }, "git_tag_delete": { "return": { "isErrorCode": true @@ -2213,16 +2264,34 @@ ] }, "tree": { + "selfFreeing": true, "functions": { - "git_tree_entry_free": { - "ignore": true + "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 } @@ -2249,6 +2318,10 @@ } } }, + "tree_entry": { + "dupFunction": "git_tree_entry_dup", + "freeFunctionName": "git_tree_entry_free" + }, "writestream": { "cType": "git_writestream", "needsForwardDeclaration": false 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/scripts/helpers.js b/generate/scripts/helpers.js index 5383bff58..6e1f33613 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,11 @@ var Helpers = { typeDef.filename = typeDef.typeName; typeDef.isLibgitType = true; 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) { @@ -241,7 +249,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 +279,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; 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; +}; diff --git a/generate/templates/partials/convert_to_v8.cc b/generate/templates/partials/convert_to_v8.cc index 100885260..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 =}}, false); + 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 34d7a0126..312c2ef28 100644 --- a/generate/templates/templates/class_content.cc +++ b/generate/templates/templates/class_content.cc @@ -24,15 +24,34 @@ using namespace v8; using namespace node; {% if cType %} - {{ cppClassName }}::{{ cppClassName }}({{ cType }} *raw, bool selfFreeing) { - this->raw = raw; + {{ 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 %} + selfFreeing = true; + } else { + 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 +96,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); @@ -92,16 +114,28 @@ 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) { + info.GetReturnValue().Set(SelfFreeingInstanceCount); + } + + NAN_METHOD({{ cppClassName }}::GetNonSelfFreeingConstructedCount) { + info.GetReturnValue().Set(NonSelfFreeingConstructedCount); } {{ cType }} *{{ cppClassName }}::GetValue() { @@ -147,3 +181,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..cc77f52b3 100644 --- a/generate/templates/templates/class_header.h +++ b/generate/templates/templates/class_header.h @@ -40,12 +40,16 @@ 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(); void ClearValue(); - static Local New(const {{ cType }} *raw, bool selfFreeing); + static Local New(const {{ cType }} *raw, bool selfFreeing, bool shouldDuplicate = false); {%endif%} bool selfFreeing; @@ -81,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%} @@ -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..e770a0c1e 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,55 @@ 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); + }); + }); + + 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); + }); });