Skip to content

Commit 206d27d

Browse files
srajkosrajko
committed
Merge pull request nodegit#1006 from nodegit/oid-leak-fix
Oid leak fix
2 parents f5280aa + 0f9e542 commit 206d27d

File tree

6 files changed

+144
-26
lines changed

6 files changed

+144
-26
lines changed

‎generate/input/descriptor.json‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,11 @@
7373
{
7474
"annotated_commit": {
7575
"functions": {
76+
"git_annotated_commit_id": {
77+
"return": {
78+
"ownedByThis": true
79+
}
80+
}
7681
}
7782
},
7883
"attr": {
@@ -1002,6 +1007,11 @@
10021007
"git_index_add_frombuffer": {
10031008
"ignore": true
10041009
},
1010+
"git_index_checksum": {
1011+
"return": {
1012+
"ownedByThis": true
1013+
}
1014+
},
10051015
"git_index_clear": {
10061016
"isAsync": true,
10071017
"return": {
@@ -1198,6 +1208,11 @@
11981208
"git_indexer_append": {
11991209
"ignore": true
12001210
},
1211+
"git_indexer_hash": {
1212+
"return": {
1213+
"ownedByThis": true
1214+
}
1215+
},
12011216
"git_indexer_new": {
12021217
"ignore": true
12031218
}
@@ -1277,6 +1292,11 @@
12771292
}
12781293
}
12791294
},
1295+
"git_note_id": {
1296+
"return": {
1297+
"ownedByThis": true
1298+
}
1299+
},
12801300
"git_note_remove": {
12811301
"isAsync": true,
12821302
"return": {
@@ -1293,6 +1313,11 @@
12931313
},
12941314
"object": {
12951315
"functions": {
1316+
"git_object_id": {
1317+
"return": {
1318+
"ownedByThis": true
1319+
}
1320+
},
12961321
"git_object_short_id": {
12971322
"args": {
12981323
"out": {
@@ -1404,6 +1429,11 @@
14041429
"cppClassName": "Wrapper",
14051430
"jsClassName": "Buffer"
14061431
}
1432+
},
1433+
"git_odb_object_id": {
1434+
"return": {
1435+
"ownedByThis": true
1436+
}
14071437
}
14081438
},
14091439
"dependencies": [
@@ -1418,6 +1448,7 @@
14181448
"ignore": true
14191449
},
14201450
"oid": {
1451+
"selfFreeing": "true",
14211452
"cpyFunction": "git_oid_cpy",
14221453
"freeFunctionName": "free",
14231454
"shouldAlloc": true,
@@ -1479,6 +1510,11 @@
14791510
"git_packbuilder_foreach": {
14801511
"ignore": true
14811512
},
1513+
"git_packbuilder_hash": {
1514+
"return": {
1515+
"ownedByThis": true
1516+
}
1517+
},
14821518
"git_packbuilder_new": {
14831519
"isAsync": false
14841520
},
@@ -1722,6 +1758,20 @@
17221758
"needsForwardDeclaration": false,
17231759
"ignore": true
17241760
},
1761+
"reflog_entry": {
1762+
"functions": {
1763+
"git_reflog_entry_id_new": {
1764+
"return": {
1765+
"ownedByThis": true
1766+
}
1767+
},
1768+
"git_reflog_entry_id_old": {
1769+
"return": {
1770+
"ownedByThis": true
1771+
}
1772+
}
1773+
}
1774+
},
17251775
"refspec": {
17261776
"cType": "git_refspec",
17271777
"functions": {
@@ -2191,6 +2241,11 @@
21912241
"isErrorCode": true
21922242
}
21932243
},
2244+
"git_submodule_head_id": {
2245+
"return": {
2246+
"ownedByThis": true
2247+
}
2248+
},
21942249
"git_submodule_init": {
21952250
"isAsync": true,
21962251
"return": {
@@ -2211,6 +2266,16 @@
22112266
"type": "int"
22122267
}
22132268
},
2269+
"git_submodule_index_id": {
2270+
"return": {
2271+
"ownedByThis": true
2272+
}
2273+
},
2274+
"git_submodule_wd_id": {
2275+
"return": {
2276+
"ownedByThis": true
2277+
}
2278+
},
22142279
"git_submodule_location": {
22152280
"isAsync": true,
22162281
"args": {

‎generate/input/libgit2-supplement.json‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,15 @@
268268
"git_patch_convenient_from_diff"
269269
]
270270
],
271+
[
272+
"reflog_entry",
273+
[
274+
"git_reflog_entry_committer",
275+
"git_reflog_entry_id_new",
276+
"git_reflog_entry_id_old",
277+
"git_reflog_entry_message"
278+
]
279+
],
271280
[
272281
"revwalk",
273282
[
@@ -705,6 +714,14 @@
705714
"git_merge_head_id"
706715
]
707716
},
717+
"reflog": {
718+
"functions": [
719+
"git_reflog_entry_committer",
720+
"git_reflog_entry_id_new",
721+
"git_reflog_entry_id_old",
722+
"git_reflog_entry_message"
723+
]
724+
},
708725
"status": {
709726
"functions": [
710727
"git_status_list_entrycount",

‎generate/scripts/helpers.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,7 @@ var Helpers = {
220220
field.jsFunctionName = utils.camelCase(field.name);
221221
field.cppClassName = Helpers.cTypeToCppName(field.type);
222222
field.jsClassName = utils.titleCase(Helpers.cTypeToJsName(field.type));
223+
field.ownedByThis = true;
223224

224225
if (Helpers.isCallbackFunction(field.cType)) {
225226
Helpers.processCallback(field);

‎test/tests/commit.js‎

Lines changed: 6 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ var promisify = require("promisify-node");
44
var fse = promisify(require("fs-extra"));
55

66
var garbageCollect = require("../utils/garbage_collect.js");
7+
var leakTest = require("../utils/leak_test");
78

89
var local = path.join.bind(path, __dirname);
910

@@ -367,11 +368,11 @@ describe("Commit", function() {
367368
}).then(function(reflog) {
368369
var reflogEntry = reflog.entryByIndex(0);
369370
assert.equal(
370-
NodeGit.Reflog.entryMessage(reflogEntry),
371+
reflogEntry.message(),
371372
customReflogMessage
372373
);
373374
assert.equal(
374-
NodeGit.Reflog.entryIdNew(reflogEntry).toString(),
375+
reflogEntry.idNew().toString(),
375376
oid
376377
);
377378
// only setTarget should have added to the entrycount
@@ -627,30 +628,9 @@ describe("Commit", function() {
627628
it("does not leak", function() {
628629
var test = this;
629630

630-
garbageCollect();
631-
var Commit = NodeGit.Commit;
632-
var startSelfFreeingCount = Commit.getSelfFreeingInstanceCount();
633-
var startNonSelfFreeingCount = Commit.getNonSelfFreeingConstructedCount();
634-
635-
var resolve;
636-
var promise = new Promise(function(_resolve) { resolve = _resolve; });
637-
638-
NodeGit.Commit.lookup(test.repository, oid)
639-
.then(function() {
640-
// get out of this promise chain to help GC get rid of the commit
641-
setTimeout(resolve, 0);
642-
});
643-
644-
return promise
645-
.then(function() {
646-
garbageCollect();
647-
var endSelfFreeingCount = Commit.getSelfFreeingInstanceCount();
648-
var endNonSelfFreeingCount = Commit.getNonSelfFreeingConstructedCount();
649-
// any new self-freeing commits should have been freed
650-
assert.equal(startSelfFreeingCount, endSelfFreeingCount);
651-
// no new non-self-freeing commits should have been constructed
652-
assert.equal(startNonSelfFreeingCount, endNonSelfFreeingCount);
653-
});
631+
return leakTest(NodeGit.Commit, function() {
632+
return NodeGit.Commit.lookup(test.repository, oid);
633+
});
654634
});
655635

656636
it("duplicates signature", function() {

‎test/tests/oid.js‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ var assert = require("assert");
22
var path = require("path");
33
var local = path.join.bind(path, __dirname);
44

5+
var leakTest = require("../utils/leak_test");
6+
57
describe("Oid", function() {
68
var NodeGit = require("../../");
79
var Oid = NodeGit.Oid;
@@ -70,4 +72,24 @@ describe("Oid", function() {
7072
var oid2 = Oid.fromString("13c633665257696a3800b0a39ff636b4593f918f");
7173
assert(!this.oid.equal(oid2));
7274
});
75+
76+
it("does not leak constructed Oid", function() {
77+
return leakTest(Oid, function() {
78+
return Promise.resolve(
79+
Oid.fromString("13c633665257696a3800b0a39ff636b4593f918f")
80+
);
81+
});
82+
});
83+
84+
it("does not leak owned Oid", function() {
85+
return leakTest(Oid, function() {
86+
return NodeGit.Repository.open(local("../repos/workdir"))
87+
.then(function(repo) {
88+
return NodeGit.Commit.lookup(repo, oid);
89+
})
90+
.then(function(commit) {
91+
return commit.id();
92+
});
93+
});
94+
});
7395
});

‎test/utils/leak_test.js‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
var assert = require("assert");
2+
3+
var garbageCollect = require("./garbage_collect");
4+
5+
function leakTest(Type, getInstance) {
6+
garbageCollect();
7+
var startSelfFreeingCount = Type.getSelfFreeingInstanceCount();
8+
var startNonSelfFreeingCount = Type.getNonSelfFreeingConstructedCount();
9+
10+
var resolve;
11+
var promise = new Promise(function(_resolve) { resolve = _resolve; });
12+
13+
getInstance()
14+
.then(function() {
15+
var selfFreeingCount = Type.getSelfFreeingInstanceCount();
16+
assert.equal(startSelfFreeingCount + 1, selfFreeingCount);
17+
// get out of this promise chain to help GC get rid of the commit
18+
setTimeout(resolve, 0);
19+
});
20+
21+
return promise
22+
.then(function() {
23+
garbageCollect();
24+
var endSelfFreeingCount = Type.getSelfFreeingInstanceCount();
25+
var endNonSelfFreeingCount = Type.getNonSelfFreeingConstructedCount();
26+
// any new self-freeing commits should have been freed
27+
assert.equal(startSelfFreeingCount, endSelfFreeingCount);
28+
// no new non-self-freeing commits should have been constructed
29+
assert.equal(startNonSelfFreeingCount, endNonSelfFreeingCount);
30+
});
31+
}
32+
33+
module.exports = leakTest;

0 commit comments

Comments
 (0)