Skip to content
This repository was archived by the owner on Dec 1, 2024. It is now read-only.
/ leveldown Public archive
This repository was archived by the owner on Dec 1, 2024. It is now read-only.

weak reference cleanup #667

@gabrielschulhof

Description

@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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    help wantedExtra attention is neededrefactorRequires or pertains to refactoring

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions