Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix segfaults on close and gc #597

Merged
merged 4 commits into from
Mar 9, 2019
Merged

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Feb 24, 2019

Closes #589, closes #157.

@vweevers
Copy link
Member Author

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

binding.cc Show resolved Hide resolved
@vweevers
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

lion_king_rafiki_throw

@ralphtheninja
Copy link
Member

Aaah now I see what draft means. Nice feature.

@vweevers
Copy link
Member Author

vweevers commented Feb 24, 2019

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
Copy link
Member Author

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.

@@ -506,6 +520,7 @@ struct Iterator {
if (gte_ != NULL) {
delete gte_;
}
delete options_;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

binding.cc Outdated Show resolved Hide resolved
binding.cc Outdated Show resolved Hide resolved
@vweevers
Copy link
Member Author

vweevers commented Mar 1, 2019

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.

@vweevers vweevers force-pushed the fix-segfaults-on-close-and-gc branch from f8d97b9 to 47fc4db Compare March 1, 2019 23:22
@vweevers vweevers force-pushed the fix-segfaults-on-close-and-gc branch from 47fc4db to 195c78c Compare March 1, 2019 23:25
@vweevers vweevers marked this pull request as ready for review March 1, 2019 23:26
@vweevers vweevers requested review from filoozom and peakji March 2, 2019 09:34
@vweevers vweevers mentioned this pull request Mar 2, 2019
Copy link
Member

@peakji peakji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vweevers vweevers merged commit d84b084 into master Mar 9, 2019
@vweevers vweevers deleted the fix-segfaults-on-close-and-gc branch March 9, 2019 09:15
@chjj
Copy link
Contributor

chjj commented Mar 14, 2019

@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
Copy link
Member

It's a good idea.

@vweevers
Copy link
Member Author

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
Copy link
Member Author

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

@vweevers
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate segfault Segfault on close with pending put
4 participants