Close database on environment exit

Closes #667, closes #755.

Author: vweevers

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<uint32_t, Iterator*> iterators = database->iterators_;
+    std::map<uint32_t, Iterator*>::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();

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')
+    })
+  })
+}

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))

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) {

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)
     }
   })
 }