Fix segfaults on close and gc

[vweevers]

Closes #589, closes #157.

[vweevers]

@ralphtheninja this is what a draft PR looks like :)

[vweevers]

Node 10 on Linux failed:

# test non-ended iterator
ok 851 no error from open()
ok 852 no error from batch()
ok 853 no error from next()
ok 854 correct key
ok 855 correct value
ok 856 no error from done()
node: ../deps/leveldb/leveldb-1.20/db/version_set.cc:798: leveldb::VersionSet::~VersionSet(): Assertion `dummy_versions_.next_ == &dummy_versions_' failed.

I'll see if I can replicate it.

[vweevers]

If I repeat the test non-ended iterator test lots of times I eventually hit this node.js assertion but not the LevelDB assertion.

[vweevers]

[ralphtheninja]

Aaah now I see what draft means. Nice feature.

[vweevers]

This still occasionally fails. The current solution is an open invitation to race issues.

@ralphtheninja Instead, could we somehow prevent GC of Iterator objects? Until they are ended (by way of it.end() of db.close()). Ideally, the EndWorker is the only code path that removes the Iterator from the map. To achieve that, we must ensure that an Iterator is not destroyed before then - but seeing as it's tied to the lifetime of the JS object, we'd have to keep track of iterators in JS-land? Ugh.

[vweevers]

Note to self (I'll pick this up again end of week): napi_create_reference should do the trick. Keep the reference count at 1, until the iterator is ended.

[vweevers]

Without this, node --expose-gc test/leak-tester-iterators shows endlessly growing memory, also on master.

[vweevers]

The last commit (f8d97b9) is probably too much for this PR (because it is not related to iterators, but fixes #157) - it was something I wanted to explore. I'll revert it later (as well as squash the other commits).

Edit: actually I quite like it, so I'm leaving it for review. In follow-up PRs we can then easily apply the fix to other operations, by swapping BaseWorker for PriorityWorker. That would close #32.

[peakji]

Nice work @vweevers !

I've just ran into the exact same issue last week, although I'm still getting used to NAPI, I can confirm it is fixed right now, so LGTM.

[chjj]

@vweevers @ralphtheninja -- one thought I've had for maybe the past couple years now is that we could "auto end" iterators on GC.

This probably belongs in a separate issue, but it would allow for nice clean code with a promise wrapper. e.g.

for await (const [key, value] of db.iterator(options)) 
  console.log([key, value]);

As opposed to:

const iter = db.iterator(options);
try {
  for await (const [key, value] of iter)
    console.log([key, value]);
} finally {
  await iter.end();
}

The former is currently impossible to do since the body of the loop may throw an error, which the async iterator cannot account for.

What do you guys think?

[ralphtheninja]

It's a good idea.

[vweevers]

I don't think the lack of syntactic sugar is a decisive argument. Note there is an ECMAScript proposal for explicit resource management. That said, I don't have a good argument for preserving .end() other than a slight personal preference for knowing and controlling the resources your app uses.

If we can make it happen, great!

[vweevers]

While this part of leveldown is still fresh in my head, I'll play around a bit.

[vweevers]

I think I have a viable, lock-free strategy:

• Make iterator.end() a noop in JS-land
• Which means EndWorker can only be called on db.close(), so remove EndWorker, instead do that work in CloseWorker
• Make NextWorker a PriorityWorker, so that it defers closing (instead of deferring EndWorker)
• Maintain a reference count per iterator, initially 0
• iterator.next() increases the refcount to prevent GC while nexting
• db.close() increases the refcounts of all iterators, to prevent GC until CloseWorker executes.