From 70da60acf6e938ffdafd47afbb1ba45a91a99c93 Mon Sep 17 00:00:00 2001
From: Matthew Keil <me@matthewkeil.com>
Date: Thu, 9 Nov 2023 17:00:24 +0300
Subject: [PATCH] refactor: update multithreading handling per PR

---
 binding.cc                  | 41 ++++++++++++++++++-------------------
 test/lock-test.js           |  6 +-----
 test/multithreading-test.js | 12 ++---------
 3 files changed, 23 insertions(+), 36 deletions(-)

diff --git a/binding.cc b/binding.cc
index df23a3b..015b283 100644
--- a/binding.cc
+++ b/binding.cc
@@ -33,7 +33,6 @@ struct LevelDbHandle
 {
   leveldb::DB *db;
   size_t open_handle_count;
-  bool multithreading;
 };
 static std::mutex handles_mutex;
 // only access this when protected by the handles_mutex!
@@ -635,25 +634,27 @@ struct Database {
 leveldb::Status threadsafe_open(const leveldb::Options &options,
                                 bool multithreading,
                                 Database &db_instance) {
+  // Bypass lock and handles if multithreading is disabled
+  if (!multithreading) {
+    return leveldb::DB::Open(options, db_instance.location_, &db_instance.db_);
+  }
+
   std::unique_lock<std::mutex> lock(handles_mutex);
 
   auto it = db_handles.find(db_instance.location_);
   if (it == db_handles.end()) {
-    // Database not opened yet for this location
-    LevelDbHandle handle = {nullptr, 0, multithreading};
+    // Database not opened yet for this location, unless it was with
+    // multithreading disabled, in which case we're expected to fail here.
+    LevelDbHandle handle = {nullptr, 0};
     leveldb::Status status = leveldb::DB::Open(options, db_instance.location_, &handle.db);
-    if (!status.ok()) {
-      return status;
-    }
-    handle.open_handle_count++;
-    db_instance.db_ = handle.db;
-    db_handles[db_instance.location_] = handle;
-    return leveldb::Status::OK();
-  }
 
-  if (!(it->second.multithreading && multithreading)) {
-    // Database already opened for this location that disallows multithreading
-    return leveldb::Status::InvalidArgument("Database already opened. Must set multithreading flag to true for all instances");
+    if (status.ok()) {
+      handle.open_handle_count++;
+      db_instance.db_ = handle.db;
+      db_handles[db_instance.location_] = handle;
+    }
+    
+    return status;
   }
 
   ++(it->second.open_handle_count);
@@ -664,20 +665,18 @@ leveldb::Status threadsafe_open(const leveldb::Options &options,
 
 leveldb::Status threadsafe_close(Database &db_instance) {
   std::unique_lock<std::mutex> lock(handles_mutex);
-  db_instance.db_ = NULL;  // ensure db_ pointer is nullified in Database instance
 
   auto it = db_handles.find(db_instance.location_);
   if (it == db_handles.end()) {
-    // this should never happen in theory but silently fail and return OK to
-    // prevent segfault if it does
-    return leveldb::Status::OK();
-  } 
-
-  if (--(it->second.open_handle_count) == 0) {
+    // Was not opened with multithreading enabled
+    delete db_instance.db_;
+  } else if (--(it->second.open_handle_count) == 0) {
     delete it->second.db;
     db_handles.erase(it);
   }
 
+  // ensure db_ pointer is nullified in Database instance
+  db_instance.db_ = NULL;
   return leveldb::Status::OK();
 }
 
diff --git a/test/lock-test.js b/test/lock-test.js
index 7991b91..cbc0aeb 100644
--- a/test/lock-test.js
+++ b/test/lock-test.js
@@ -18,11 +18,7 @@ test('lock held by same process', async function (t) {
     await db2.open()
   } catch (err) {
     t.is(err.code, 'LEVEL_DATABASE_NOT_OPEN', 'second instance failed to open')
-    t.is(
-      err.cause.message,
-      'Invalid argument: Database already opened. Must set multithreading flag to true for all instances',
-      'second instance got lock error'
-    )
+    t.is(err.cause.code, 'LEVEL_LOCKED', 'second instance got lock error')
   }
 
   return db1.close()
diff --git a/test/multithreading-test.js b/test/multithreading-test.js
index 872516a..74e58d1 100644
--- a/test/multithreading-test.js
+++ b/test/multithreading-test.js
@@ -34,11 +34,7 @@ test('check multithreading flag works as expected', async function (t) {
     await db2.open({ multithreading: true })
   } catch (err) {
     t.is(err.code, 'LEVEL_DATABASE_NOT_OPEN', 'second instance failed to open')
-    t.is(
-      err.cause.message,
-      'Invalid argument: Database already opened. Must set multithreading flag to true for all instances',
-      'second instance got lock error'
-    )
+    t.is(err.cause.code, 'LEVEL_LOCKED', 'second instance got lock error')
   }
   await db1.close()
 
@@ -55,11 +51,7 @@ test('check multithreading flag works as expected', async function (t) {
     await db4.open({ location, multithreading: false })
   } catch (err) {
     t.is(err.code, 'LEVEL_DATABASE_NOT_OPEN', 'fourth instance failed to open')
-    t.is(
-      err.cause.message,
-      'Invalid argument: Database already opened. Must set multithreading flag to true for all instances',
-      'fourth instance got lock error'
-    )
+    t.is(err.cause.code, 'LEVEL_LOCKED', 'second instance got lock error')
   }
   await db1.close()
   await db2.close()