From c7e98d3379ce3ad241d8c60160582416fe06fe86 Mon Sep 17 00:00:00 2001 From: Alan Shaw Date: Fri, 3 Jan 2020 22:25:52 +0000 Subject: [PATCH 1/2] 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 | 7 ++++- test/fixtures/test-level-iterator-destroy.js | 18 ++++++++++++ test/node.js | 30 ++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/test-level-iterator-destroy.js diff --git a/src/index.js b/src/index.js index a3760f7..7ac8bab 100644 --- a/src/index.js +++ b/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 } }) }) }), diff --git a/test/fixtures/test-level-iterator-destroy.js b/test/fixtures/test-level-iterator-destroy.js new file mode 100644 index 0000000..edfc018 --- /dev/null +++ b/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) + } +} + +// 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() diff --git a/test/node.js b/test/node.js index 9c11ad0..2cf0115 100644 --- a/test/node.js +++ b/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() + }) + }) }) From 81476a8cdec7698b5d1ebfc8e065356f1916e819 Mon Sep 17 00:00:00 2001 From: Alan Shaw Date: Fri, 3 Jan 2020 22:29:25 +0000 Subject: [PATCH 2/2] chore: appease linter --- test/fixtures/test-level-iterator-destroy.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/test-level-iterator-destroy.js b/test/fixtures/test-level-iterator-destroy.js index edfc018..a71a3d5 100644 --- a/test/fixtures/test-level-iterator-destroy.js +++ b/test/fixtures/test-level-iterator-destroy.js @@ -8,7 +8,7 @@ async function testLevelIteratorDestroy () { 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) + console.log(d) // eslint-disable-line no-console } }