Comments, cleanup

Author: cb1kenobi

src/binding/database.cpp

@@ -936,6 +936,10 @@ napi_value Database::Open(napi_env env, napi_callback_info info) {
 
 	try {
 		(*dbHandle)->open(path, dbHandleOptions);
+
+		// now that the database is open and the dbHandle has a reference to
+		// the descriptor, we can attach the database instance's smart_ptr to
+		// the descriptor so it gets cleaned up when the descriptor is closed
 		(*dbHandle)->descriptor->attach(*dbHandle);
 	} catch (const std::exception& e) {
 		DEBUG_LOG("%p Database::Open Error: %s\n", dbHandle->get(), e.what());

src/binding/db_descriptor.cpp

@@ -194,6 +194,9 @@ void DBDescriptor::close() {
 
 /**
  * Registers a database resource to be closed when the descriptor is closed.
+ *
+ * Important: The closable must be same smart_ptr that is napi-wrapped and
+ * bound to the JavaScript class counterpart.
  */
 void DBDescriptor::attach(std::shared_ptr<Closable> closable) {
 	std::lock_guard<std::mutex> lock(this->txnsMutex);

src/binding/db_handle.cpp

@@ -103,6 +103,10 @@ void DBHandle::open(const std::string& path, const DBOptions& options) {
 	this->disableWAL = options.disableWAL;
 	this->path = path;
 
+	// Note: We cannot attach this handle to the descriptor because we don't
+	// have the smart pointer to the dbHandle instance, so the caller needs to
+	// do it.
+
 	// at this point, the DBDescriptor has at least 2 refs: the registry and this handle
 }
 

src/binding/db_registry.cpp

@@ -22,7 +22,9 @@ void DBRegistry::CloseDB(const std::shared_ptr<DBHandle> handle) {
 		return;
 	}
 
+#ifdef DEBUG
 	DBRegistry::DebugLogDescriptorRefs();
+#endif
 
 	if (!handle->descriptor) {
 		DEBUG_LOG("%p DBRegistry::CloseDB Database not opened\n", instance.get());
@@ -73,15 +75,15 @@ void DBRegistry::CloseDB(const std::shared_ptr<DBHandle> handle) {
 /**
  * Debug log the reference count of all descriptors in the registry.
  */
-void DBRegistry::DebugLogDescriptorRefs() {
 #ifdef DEBUG
+void DBRegistry::DebugLogDescriptorRefs() {
 	std::lock_guard<std::mutex> lock(instance->databasesMutex);
 	DEBUG_LOG("DBRegistry::DebugLogDescriptorRefs %d descriptor%s in registry:\n", instance->databases.size(), instance->databases.size() == 1 ? "" : "s");
 	for (auto& [path, entry] : instance->databases) {
 		DEBUG_LOG("  %p for \"%s\" (ref count = %ld)\n", entry.descriptor.get(), path.c_str(), entry.descriptor.use_count());
 	}
-#endif
 }
+#endif
 
 /**
  * Destroy a RocksDB database.

src/binding/db_registry.h

@@ -64,7 +64,9 @@ class DBRegistry final {
 
 public:
 	static void CloseDB(const std::shared_ptr<DBHandle> handle);
+#ifdef DEBUG
 	static void DebugLogDescriptorRefs();
+#endif
 	static void DestroyDB(const std::string& path);
 	static void Init(napi_env env, napi_value exports);
 	static std::unique_ptr<DBHandleParams> OpenDB(const std::string& path, const DBOptions& options);