weak reference cleanup

[gabrielschulhof]

At https://github.com/Level/leveldown/blob/master/test/stack-blower.js a bunch of iterators are created until the process runs out of stack space. The process exits gracefully, reporting the number of recursions it was able to perform.

Currently, neither is the database closed, nor are any of the iterators ended when the process exits. That is, iterator_end_do() is not called and neither is FinalizeIterator().

This means that, if the addon were to run in a worker thread and used incorrectly, these objects would be left over when the thread exited. If, furthermore, the worker threads were part of a long-running application that repeatedly spawned such worker threads, these omitted finalizations would build up to a memory leak.

To address such problems with addons, I'm working on nodejs/node#28428, which would call finalizers on outstanding references at environment exit. As I was testing leveldown against the PR I noticed that the above-linked test threw an assertion error, because the Iterator's destructor was being called without the iterator being ended first, triggering the assertion assert(ended_);.

One solution for the test is to close the database before doing the process.send() and exiting.

In general, I'm not sure what is the best way to tear things down if the Node.js environment exits without the application first cleaning up any open database handles and/or iterators it may have.

Some solutions that come to mind:

• Start using napi_add_environment_cleanup_hook(), which is available on all Node.js LTS versions. It would inform leveldown that the environment is being torn down, and that, as a result, all outstanding handles should be closed, because user code will certainly not close them.

• Change the test to expect a non-clean exit of the stack-blower child process because of the assertion. This would force users to clean up after themselves lest their process exit with an abort() after n-api: render finalizers as env cleanup hooks nodejs/node#28428 lands.

• Add db.close() to stack-blower.js and document that handles need to be closed before process exit in order to avoid an abort().

• Remove the assertion and end the iterator upon destruction. To me it feels like this might not be the right path, otherwise it would already have been done, and that requiring users to explicitly end the iterator is done by design.

I don't know enough about the semantics of leveldb to know which if any of these solutions are correct, so I'm hoping we can discuss how leveldb might clean up after itself at environment exit or how it might indicate to users that they need to clean up their open handles at environment exit.

[vweevers]

Thanks for reaching out!

and that requiring users to explicitly end the iterator is done by design

It is. We've explored automatically ending on GC (#601) and concluded it degraded performance (because holding too many LevelDB snapshots negatively affects compaction of new keys) and wasn't worth the code complexity.

This means that, if the addon were to run in a worker thread

Because leveldown already does most of its work in async workers (napi_async_work), let me first ask what benefits a node.js worker thread would have?

[gabrielschulhof]

@vweevers I'm thinking of the case where leveldown is part of a larger, multi-threaded, Node.js application where threads come and go and each one uses leveldown (improperly 🙂).

Another scenario where the process outlives the Node.js environment is in applications where Node.js is embedded. For example, in Electron, multiple Node.js environments can be created both at the same time and in sequence. As each environment is torn down, it should properly clean up after itself. If leveldown is part of the Node.js application that runs in that environment, it too should release all memory it allocated during its life cycle.

Basically, as long as the process outlives the environment and ends up creating new Node.js environments later, the native addons loaded into the process must all deallocate all the resources they allocate during their life cycle, otherwise a memory leak will ensue.

[vweevers]

Start using napi_add_environment_cleanup_hook(), which is available on all Node.js LTS versions. It would inform leveldown that the environment is being torn down, and that, as a result, all outstanding handles should be closed, because user code will certainly not close them.

Might be tricky because closing the db is currently asynchronous. Does the hook support that?

(If you're reading along: see napi_add_env_cleanup_hook)

[vweevers]

Side note regarding Electron: we've been suggesting to users to keep leveldown in the main process, not in renderer processes, because LevelDB can only be accessed by a single process at a time.

[gabrielschulhof]

I don't believe the hook supports async operation. However, given that the environment is about to exit, can the database_->CloseDatabase() not be called directly, on the main thread?

[vweevers]

We could, but it's currently tied into other logic. Before closing, we also wait for any pending operations (in async workers) to finish, and we have to end iterators. So we would need to refactor that, in such a way that we can initiate the closing both asynchronously and synchronously.

[vweevers]

I think for now, I want to say we don't support these scenarios. Mostly because of time constraints. I'll merge & release the PR so that you're not blocked by it @gabrielschulhof.

If anyone wants to take a stab at implementing the hook, go for it!

[gabrielschulhof]

Given that problems only arise out of incorrect usage, I guess merging my PR into core would only expose the bugs that are already there.

[vweevers]

@gabrielschulhof How should we deal with async workers? Let's say a user does:

// creates an async worker to open the LevelDB database
db.open(callback)
process.exit()

What does that do to pending async workers? Are they cancelled or terminated before or after napi_add_env_cleanup_hook?

[gabrielschulhof]

@vweevers I believe the cleanup hooks are fired right before the environment exits, meaning that async workers will have been processed one way or another. @addaleax knows best though.

[addaleax]

I believe the cleanup hooks are fired right before the environment exits, meaning that async workers will have been processed one way or another

@vweevers @gabrielschulhof Node.js will wait for already-scheduled napi_async_work items to finish before cleanup hooks are run (unless they are scheduled from a cleanup hook – That’s actually kind of the only way that N-API currently provides for actually doing async cleanup on Environment teardown).

[ronag]

Shouldn't we add a process.emitWarning to leveldown when running inside a worker. Until this has been resolved?

[vweevers]

How is such a warning surfaced in workers? A worker's stderr is by default piped to the parent, correct?