diff --git a/binding.cc b/binding.cc index 476326c9..2efa2605 100644 --- a/binding.cc +++ b/binding.cc @@ -726,6 +726,33 @@ NAPI_METHOD(db_init) { return result; } +// Hook for when the environment exits. +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 (and within) 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. + 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(); + } +} + /** * Worker class for opening a database. */ @@ -791,6 +818,11 @@ NAPI_METHOD(db_open) { database->blockCache_ = leveldb::NewLRUCache(cacheSize); + // Register a hook for when the environment exits. 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 that time. + napi_add_env_cleanup_hook(env, env_cleanup_hook, database); + napi_value callback = argv[3]; OpenWorker* worker = new OpenWorker(env, database, callback, location, createIfMissing, errorIfExists, @@ -833,6 +865,12 @@ NAPI_METHOD(db_close) { napi_value callback = argv[1]; CloseWorker* worker = new CloseWorker(env, database, callback); + // Remove hook for when the environment exits. Don't need it anymore + // because we're closing here and although that is asynchronous, the + // environment will wait for already-scheduled napi_async_work items + // to finish before exiting. + napi_remove_env_cleanup_hook(env, env_cleanup_hook, database); + if (!database->HasPriorityWork()) { worker->Queue(); NAPI_RETURN_UNDEFINED(); 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..17da8f9b --- /dev/null +++ b/test/env-cleanup-hook.js @@ -0,0 +1,62 @@ +'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 indicating at which step we stopped, and + // triggering an environment exit. + return process.send(step) + } + + // Create a db, expected only to be garbage-collected because + // the environment cleanup hook is not added until open(). + const db = testCommon.factory() + + if (nextStep() !== 'open') { + if (nextStep() === 'open-error') { + // If opening fails we'll still have the cleanup hook but it will 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, expected only to be garbage-collected because we + // synchronously remove the cleanup hook in close(). + 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) } }) }