Remove panic from inner connection initialization (#54)

* Remove panic from inner connection initialization

* Add clone drop test

* In UT, close conn explicitly

Author: aarashy

src/inner_connection.rs

@@ -16,25 +16,19 @@ pub struct InnerConnection {
     owned: bool,
 }
 
-impl Clone for InnerConnection {
-    fn clone(&self) -> Self {
-        unsafe { InnerConnection::new(self.db, false) }
-    }
-}
-
 impl InnerConnection {
-    #[allow(clippy::mutex_atomic)]
     #[inline]
-    pub unsafe fn new(db: ffi::duckdb_database, owned: bool) -> InnerConnection {
+    pub unsafe fn new(db: ffi::duckdb_database, owned: bool) -> Result<InnerConnection> {
         let mut con: ffi::duckdb_connection = ptr::null_mut();
         let r = ffi::duckdb_connect(db, &mut con);
         if r != ffi::DuckDBSuccess {
             ffi::duckdb_disconnect(&mut con);
-            let e = Error::DuckDBFailure(ffi::Error::new(r), Some("connect error".to_owned()));
-            // TODO: fix this
-            panic!("error {:?}", e);
+            return Err(Error::DuckDBFailure(
+                ffi::Error::new(r),
+                Some("connect error".to_owned()),
+            ));
         }
-        InnerConnection { db, con, owned }
+        Ok(InnerConnection { db, con, owned })
     }
 
     pub fn open_with_flags(c_path: &CStr, config: Config) -> Result<InnerConnection> {
@@ -47,11 +41,10 @@ impl InnerConnection {
                 ffi::duckdb_free(c_err as *mut c_void);
                 return Err(Error::DuckDBFailure(ffi::Error::new(r as u32), msg));
             }
-            Ok(InnerConnection::new(db, true))
+            InnerConnection::new(db, true)
         }
     }
 
-    #[allow(clippy::mutex_atomic)]
     pub fn close(&mut self) -> Result<()> {
         if self.db.is_null() {
             return Ok(());
@@ -71,6 +64,11 @@ impl InnerConnection {
         Ok(())
     }
 
+    /// Creates a new connection to the already-opened database.
+    pub fn try_clone(&self) -> Result<Self> {
+        unsafe { InnerConnection::new(self.db, false) }
+    }
+
     pub fn execute(&mut self, sql: &str) -> Result<()> {
         let c_str = CString::new(sql).unwrap();
         unsafe {
@@ -106,12 +104,6 @@ impl InnerConnection {
         Ok(Appender::new(conn, c_app))
     }
 
-    #[inline]
-    #[allow(dead_code)]
-    pub fn changes(&mut self) -> usize {
-        panic!("changes: not supported")
-    }
-
     #[inline]
     pub fn is_autocommit(&self) -> bool {
         true

src/lib.rs

@@ -197,16 +197,6 @@ pub struct Connection {
 
 unsafe impl Send for Connection {}
 
-impl Clone for Connection {
-    /// Open a new db connection
-    fn clone(&self) -> Self {
-        Connection {
-            db: RefCell::new(self.db.borrow().clone()),
-            path: self.path.clone(),
-        }
-    }
-}
-
 impl Connection {
     /// Open a new connection to a DuckDB database.
     ///
@@ -510,6 +500,15 @@ impl Connection {
     pub fn is_autocommit(&self) -> bool {
         self.db.borrow().is_autocommit()
     }
+
+    /// Creates a new connection to the already-opened database.
+    pub fn try_clone(&self) -> Result<Self> {
+        let inner = self.db.borrow().try_clone()?;
+        Ok(Connection {
+            db: RefCell::new(inner),
+            path: self.path.clone(),
+        })
+    }
 }
 
 impl fmt::Debug for Connection {
@@ -922,12 +921,30 @@ mod test {
 
     #[test]
     fn test_clone() -> Result<()> {
-        let owned_con = checked_memory_handle();
+        // 1. Drop the cloned connection first. The original connection should still be able to run queries.
+        {
+            let owned_con = checked_memory_handle();
+            {
+                let cloned_con = owned_con.try_clone().unwrap();
+                cloned_con.execute_batch("create table test (c1 bigint)")?;
+                cloned_con.close().unwrap();
+            }
+            owned_con.execute_batch("create table test2 (c1 bigint)")?;
+            owned_con.close().unwrap();
+        }
+
+        // 2. Close the original connection first. The cloned connection should still be able to run queries.
         {
-            let cloned_con = owned_con.clone();
-            cloned_con.execute_batch("PRAGMA VERSION")?;
+            let cloned_con = {
+                let owned_con = checked_memory_handle();
+                let clone = owned_con.try_clone().unwrap();
+                owned_con.execute_batch("create table test (c1 bigint)")?;
+                owned_con.close().unwrap();
+                clone
+            };
+            cloned_con.execute_batch("create table test2 (c1 bigint)")?;
+            cloned_con.close().unwrap();
         }
-        owned_con.close().unwrap();
         Ok(())
     }
 

src/r2d2.rs

@@ -86,7 +86,7 @@ impl r2d2::ManageConnection for DuckdbConnectionManager {
 
     fn connect(&self) -> Result<Self::Connection, Self::Error> {
         let conn = self.connection.lock().unwrap();
-        Ok(conn.clone())
+        conn.try_clone()
     }
 
     fn is_valid(&self, conn: &mut Self::Connection) -> Result<(), Self::Error> {