Close database on environment exit (#783)

Closes #667, closes #755.

Author: vweevers

binding.cc

@@ -468,15 +468,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.
  */
@@ -713,11 +704,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<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();
+  }
+}
+
+/**
+ * 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,

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

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