Fix panic when calling delete_table() for a table that is open

cberner · Nov 7, 2023 · f1dea10 · f1dea10
1 parent fc150a2
commit f1dea10
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 11 deletions.
diff --git a/src/transactions.rs b/src/transactions.rs
@@ -360,6 +360,39 @@ impl<'db> TableNamespace<'db> {
         ))
     }
 
+    #[track_caller]
+    fn inner_delete(&mut self, name: &str, table_type: TableType) -> Result<bool, TableError> {
+        if let Some(location) = self.open_tables.get(name) {
+            return Err(TableError::TableAlreadyOpen(name.to_string(), location));
+        }
+
+        self.table_tree.delete_table(name, table_type)
+    }
+
+    #[track_caller]
+    fn delete_table<'txn>(
+        &mut self,
+        transaction: &'txn WriteTransaction<'db>,
+        name: &str,
+    ) -> Result<bool, TableError> {
+        #[cfg(feature = "logging")]
+        info!("Deleting table: {}", name);
+        transaction.dirty.store(true, Ordering::Release);
+        self.inner_delete(name, TableType::Normal)
+    }
+
+    #[track_caller]
+    fn delete_multimap_table<'txn>(
+        &mut self,
+        transaction: &'txn WriteTransaction<'db>,
+        name: &str,
+    ) -> Result<bool, TableError> {
+        #[cfg(feature = "logging")]
+        info!("Deleting multimap table: {}", name);
+        transaction.dirty.store(true, Ordering::Release);
+        self.inner_delete(name, TableType::Multimap)
+    }
+
     pub(crate) fn close_table<K: RedbKey + 'static, V: RedbValue + 'static>(
         &mut self,
         name: &str,
@@ -741,14 +774,10 @@ impl<'db> WriteTransaction<'db> {
     ///
     /// Returns a bool indicating whether the table existed
     pub fn delete_table(&self, definition: impl TableHandle) -> Result<bool, TableError> {
-        #[cfg(feature = "logging")]
-        info!("Deleting table: {}", definition.name());
-        self.dirty.store(true, Ordering::Release);
         self.tables
             .lock()
             .unwrap()
-            .table_tree
-            .delete_table(definition.name(), TableType::Normal)
+            .delete_table(self, definition.name())
     }
 
     /// Delete the given table
@@ -758,14 +787,10 @@ impl<'db> WriteTransaction<'db> {
         &self,
         definition: impl MultimapTableHandle,
     ) -> Result<bool, TableError> {
-        #[cfg(feature = "logging")]
-        info!("Deleting multimap table: {}", definition.name());
-        self.dirty.store(true, Ordering::Release);
         self.tables
             .lock()
             .unwrap()
-            .table_tree
-            .delete_table(definition.name(), TableType::Multimap)
+            .delete_multimap_table(self, definition.name())
     }
 
     /// List all the tables

diff --git a/tests/basic_tests.rs b/tests/basic_tests.rs
@@ -1,7 +1,7 @@
 use redb::backends::InMemoryBackend;
 use redb::{
     Database, MultimapTableDefinition, MultimapTableHandle, Range, ReadableTable, RedbKey,
-    RedbValue, TableDefinition, TableHandle, TypeName,
+    RedbValue, TableDefinition, TableError, TableHandle, TypeName,
 };
 use std::cmp::Ordering;
 #[cfg(not(target_os = "wasi"))]
@@ -812,6 +812,22 @@ fn delete() {
     assert_eq!(table.len().unwrap(), 1);
 }
 
+#[test]
+fn delete_open_table() {
+    let tmpfile = create_tempfile();
+    let db = Database::create(tmpfile.path()).unwrap();
+    let write_txn = db.begin_write().unwrap();
+    {
+        let table = write_txn.open_table(STR_TABLE).unwrap();
+        assert!(matches!(
+            write_txn.delete_table(STR_TABLE).unwrap_err(),
+            TableError::TableAlreadyOpen(_, _)
+        ));
+        drop(table);
+    }
+    write_txn.commit().unwrap();
+}
+
 #[test]
 fn no_dirty_reads() {
     let tmpfile = create_tempfile();