fix: leveldb iterator memory leak (#26)

* fix: leveldb iterator memory leak

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

src/index.js

@@ -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) {
+          return li.end(err => {
+            if (err) return reject(err)
+            resolve({ done: true })
+          })
+        }
         resolve({ done: false, value: { key, value } })
       })
     }),

test/fixtures/test-level-iterator-destroy.js

@@ -0,0 +1,18 @@
+'use strict'
+
+const { utils } = require('interface-datastore')
+const LevelStore = require('../../src')
+
+async function testLevelIteratorDestroy () {
+  const store = new LevelStore(utils.tmpdir(), { db: require('level') })
+  await store.open()
+  await store.put(`/test/key${Date.now()}`, Buffer.from(`TESTDATA${Date.now()}`))
+  for await (const d of store.query({})) {
+    console.log(d) // eslint-disable-line no-console
+  }
+}
+
+// Will exit with:
+// > Assertion failed: (ended_), function ~Iterator, file ../binding.cc, line 546.
+// If iterator gets destroyed (in c++ land) and .end() was not called on it.
+testLevelIteratorDestroy()

test/node.js

@@ -10,6 +10,7 @@ const rimraf = require('rimraf')
 const { MountDatastore } = require('datastore-core')
 const CID = require('cids')
 const { promisify } = require('util')
+const childProcess = require('child_process')
 
 const LevelStore = require('../src')
 
@@ -68,4 +69,33 @@ describe('LevelDatastore', () => {
     expect(cids[0].version).to.be.eql(0)
     expect(cids).to.have.length(4)
   })
+
+  // The `.end()` method MUST be called on LevelDB iterators or they remain open,
+  // leaking memory.
+  //
+  // This test exposes this 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
+  it('should not leave iterators open and leak memory', (done) => {
+    const cp = childProcess.fork(`${__dirname}/fixtures/test-level-iterator-destroy`, { stdio: 'pipe' })
+
+    let out = ''
+    cp.stdout.on('data', d => { out += d })
+    cp.stderr.on('data', d => { out += d })
+
+    cp.on('exit', code => {
+      expect(code).to.equal(0)
+      expect(out).to.not.include('Assertion failed: (ended_)')
+      done()
+    })
+  })
 })