From 54fc5df351152dc8d7f71006069f89b9ffd19783 Mon Sep 17 00:00:00 2001 From: Vincent Weevers Date: Sun, 12 Sep 2021 11:12:38 +0200 Subject: [PATCH] Close database on environment exit (cherry picked from commit Level/leveldown@8fdcaaa3bdadec3a2badf8bf13db28fbbfa9a93c) --- binding.cc | 53 ++++++++++++++++++++++++----- test/env-cleanup-hook-test.js | 33 ++++++++++++++++++ test/env-cleanup-hook.js | 59 +++++++++++++++++++++++++++++++++ test/iterator-recursion-test.js | 3 +- test/stack-blower.js | 7 +--- 5 files changed, 138 insertions(+), 17 deletions(-) create mode 100644 test/env-cleanup-hook-test.js create mode 100644 test/env-cleanup-hook.js diff --git a/binding.cc b/binding.cc index 67ac9a81..31c17dd0 100644 --- a/binding.cc +++ b/binding.cc @@ -481,15 +481,6 @@ struct Database { uint32_t priorityWork_; }; -/** - * Runs when a Database is garbage collected. - */ -static void FinalizeDatabase (napi_env env, void* data, void* hint) { - if (data) { - delete (Database*)data; - } -} - /** * Base worker class for doing async work that defers closing the database. */ @@ -718,11 +709,55 @@ struct Iterator { napi_ref ref_; }; +/** + * Hook for when the environment exits. This hook will be called after + * already-scheduled napi_async_work items have finished, which gives us + * the guarantee that no db operations will be in-flight at this time. + */ +static void env_cleanup_hook (void* arg) { + Database* database = (Database*)arg; + + // Do everything that db_close() does but synchronously. We're expecting that GC + // did not (yet) collect the database because that would be a user mistake (not + // closing their db) made during the lifetime of the environment. That's different + // from an environment being torn down (like the main process or a worker thread) + // where it's our responsibility to clean up. Note also, the following code must + // be a safe noop if called before db_open() or after db_close(). + if (database && database->db_ != NULL) { + std::map iterators = database->iterators_; + std::map::iterator it; + + for (it = iterators.begin(); it != iterators.end(); ++it) { + Iterator* iterator = it->second; + + if (!iterator->ended_) { + iterator->ended_ = true; + iterator->IteratorEnd(); + } + } + + // Having ended the iterators (and released snapshots) we can safely close. + database->CloseDatabase(); + } +} + +/** + * Runs when a Database is garbage collected. + */ +static void FinalizeDatabase (napi_env env, void* data, void* hint) { + if (data) { + Database* database = (Database*)data; + napi_remove_env_cleanup_hook(env, env_cleanup_hook, database); + delete database; + } +} + /** * Returns a context object for a database. */ NAPI_METHOD(db_init) { Database* database = new Database(env); + napi_add_env_cleanup_hook(env, env_cleanup_hook, database); napi_value result; NAPI_STATUS_THROWS(napi_create_external(env, database, diff --git a/test/env-cleanup-hook-test.js b/test/env-cleanup-hook-test.js new file mode 100644 index 00000000..cf20143d --- /dev/null +++ b/test/env-cleanup-hook-test.js @@ -0,0 +1,33 @@ +'use strict' + +const test = require('tape') +const fork = require('child_process').fork +const path = require('path') + +// Test env_cleanup_hook at several stages of a db lifetime +addTest(['create']) +addTest(['create', 'open']) +addTest(['create', 'open', 'create-iterator']) +addTest(['create', 'open', 'create-iterator', 'close']) +addTest(['create', 'open', 'create-iterator', 'nexting']) +addTest(['create', 'open', 'create-iterator', 'nexting', 'close']) +addTest(['create', 'open', 'close']) +addTest(['create', 'open-error']) + +function addTest (steps) { + test(`cleanup on environment exit (${steps.join(', ')})`, function (t) { + t.plan(3) + + const child = fork(path.join(__dirname, 'env-cleanup-hook.js'), steps) + + child.on('message', function (m) { + t.is(m, steps[steps.length - 1], `got to step: ${m}`) + child.disconnect() + }) + + child.on('exit', function (code, sig) { + t.is(code, 0, 'child exited normally') + t.is(sig, null, 'not terminated due to signal') + }) + }) +} diff --git a/test/env-cleanup-hook.js b/test/env-cleanup-hook.js new file mode 100644 index 00000000..2c20a633 --- /dev/null +++ b/test/env-cleanup-hook.js @@ -0,0 +1,59 @@ +'use strict' + +const testCommon = require('./common') + +function test (steps) { + let step + + function nextStep () { + step = steps.shift() || step + return step + } + + if (nextStep() !== 'create') { + // Send a message triggering an environment exit + // and indicating at which step we stopped. + return process.send(step) + } + + const db = testCommon.factory() + + if (nextStep() !== 'open') { + if (nextStep() === 'open-error') { + // If opening fails the cleanup hook should be a noop. + db.open({ createIfMissing: false, errorIfExists: true }, function (err) { + if (!err) throw new Error('Expected an open() error') + }) + } + + return process.send(step) + } + + // Open the db, expected to be closed by the cleanup hook. + db.open(function (err) { + if (err) throw err + + if (nextStep() === 'create-iterator') { + // Create an iterator, expected to be ended by the cleanup hook. + const it = db.iterator() + + if (nextStep() === 'nexting') { + // This async work should finish before the cleanup hook is called. + it.next(function (err) { + if (err) throw err + }) + } + } + + if (nextStep() === 'close') { + // Close the db, after which the cleanup hook is a noop. + db.close(function (err) { + if (err) throw err + }) + } + + process.send(step) + }) +} + +test(process.argv.slice(2)) diff --git a/test/iterator-recursion-test.js b/test/iterator-recursion-test.js index fbdc4293..89eb8e38 100644 --- a/test/iterator-recursion-test.js +++ b/test/iterator-recursion-test.js @@ -27,8 +27,7 @@ test('setUp common', testCommon.setUp) // call in our Iterator to segfault. This was fixed in 2014 (commit 85e6a38). // // Today (2020), we see occasional failures in CI again. We no longer call -// node::FatalException() so there's a new reason. Possibly related to -// https://github.com/Level/leveldown/issues/667. +// node::FatalException() so there's a new reason. test.skip('try to create an iterator with a blown stack', function (t) { for (let i = 0; i < 100; i++) { t.test(`try to create an iterator with a blown stack (${i})`, function (t) { diff --git a/test/stack-blower.js b/test/stack-blower.js index 390bcbb5..3fda3de4 100644 --- a/test/stack-blower.js +++ b/test/stack-blower.js @@ -21,12 +21,7 @@ if (process.argv[2] === 'run') { try { recurse() } catch (e) { - // Closing before process exit is normally not needed. This is a - // temporary workaround for Level/leveldown#667. - db.close(function (err) { - if (err) throw err - process.send('Catchable error at depth ' + depth) - }) + process.send('Catchable error at depth ' + depth) } }) }