fix: leveldb iterator memory leak #26
Conversation
The `.end()` method MUST be called on LevelDB iterators or they remain open, leaking memory. This PR calls `.end()` on the leveldb iterator when it is done. The added test exposes the problem by causing an error to be thrown on process exit when an iterator is open AND leveldb is not closed. Normally when leveldb is closed it'll automatically clean up open iterators but if you don't close the store this error will occur: > Assertion failed: (ended_), function ~Iterator, file ../binding.cc, line 546. This is thrown by a destructor function for iterator objects that asserts the iterator has ended before cleanup. https://github.com/Level/leveldown/blob/d3453fbde4d2a8aa04d9091101c25c999649069b/binding.cc#L545 FYI: > Destructors are usually used to deallocate memory and do other cleanup for a class object and its class members when the object is destroyed. A destructor is called for a class object when that object passes out of scope or is explicitly deleted. > https://www.ibm.com/support/knowledgecenter/en/ssw_ibm_i_74/rzarg/cplr380.htm
|
This looks good but I can't get the test to fail against master - see the build here - it has the new test added but no change made to the |
|
hmm, maybe the error is only printed in nodejs 12 (this is what I was using locally). I’ll take a look. |
| @@ -158,7 +158,12 @@ function levelIteratorToIterator (li) { | |||
| next: () => new Promise((resolve, reject) => { | |||
| li.next((err, key, value) => { | |||
| if (err) return reject(err) | |||
| if (key == null) return resolve({ done: true }) | |||
| if (key == null) { | |||
There was a problem hiding this comment.
For completeness can you also implement the Generator.prototype.throw() method with similar logic?
|
|
||
| cp.on('exit', code => { | ||
| expect(code).to.equal(0) | ||
| expect(out).to.not.include('Assertion failed: (ended_)') |
There was a problem hiding this comment.
Is there a way to assert on the presence of something rather than the absence of something? Even something we add to the fixture to demonstrate that the iterator has been torn down correctly?
Otherwise this test is coupled to the internal implementation of leveldown which may change without us knowing and this test may not expose the problem it is designed to.
There was a problem hiding this comment.
Not really, the leveldb iterator is not exposed an any way and the js iterator always ends fine. Given that this doesn’t even work on Node.js 10 I think this is not a good test.
That said I’m all out of ideas for how to test it right now!
There was a problem hiding this comment.
(Still internal but at least it'd assert presence of something) you could check it._ended.
Yes, calling finalizers on exit is a recent node 12 behavior. For background, see Level/leveldown#667 and nodejs/node#28428 |
The
.end()method MUST be called on LevelDB iterators or they remain open, leaking memory.This PR calls
.end()on the leveldb iterator when it is done.The added test exposes the problem by causing an error to be thrown on process exit when an iterator is open AND leveldb is not closed.
Normally when leveldb is closed it'll automatically clean up open iterators but if you don't close the store this error will occur:
This is thrown by a destructor function for iterator objects that asserts the iterator has ended before cleanup.
https://github.com/Level/leveldown/blob/d3453fbde4d2a8aa04d9091101c25c999649069b/binding.cc#L545
FYI: