Merge pull request #378 from gwenn/callback

Fix hooks

src/functions.rs

@@ -182,7 +182,7 @@ impl<'a> ValueRef<'a> {
 }
 
 unsafe extern "C" fn free_boxed_value<T>(p: *mut c_void) {
-    let _: Box<T> = Box::from_raw(p as *mut T);
+    drop(Box::from_raw(p as *mut T));
 }
 
 /// Context is a wrapper for the SQLite function evaluation context.

src/hooks.rs

@@ -94,8 +94,9 @@ impl Connection {
     /// Register a callback function to be invoked whenever a transaction is committed.
     ///
     /// The callback returns `true` to rollback.
-    pub fn commit_hook<F>(&self, hook: F)
+    pub fn commit_hook<F>(&self, hook: Option<F>)
-        where F: FnMut() -> bool
+    where
+        F: FnMut() -> bool,
     {
         self.db.borrow_mut().commit_hook(hook);
     }
@@ -103,8 +104,9 @@ impl Connection {
     /// Register a callback function to be invoked whenever a transaction is committed.
     ///
     /// The callback returns `true` to rollback.
-    pub fn rollback_hook<F>(&self, hook: F)
+    pub fn rollback_hook<F>(&self, hook: Option<F>)
-        where F: FnMut()
+    where
+        F: FnMut(),
     {
         self.db.borrow_mut().rollback_hook(hook);
     }
@@ -118,99 +120,122 @@ impl Connection {
     ///   - the name of the database ("main", "temp", ...),
     ///   - the name of the table that is updated,
     ///   - the ROWID of the row that is updated.
-    pub fn update_hook<F>(&self, hook: F)
+    pub fn update_hook<F>(&self, hook: Option<F>)
-        where F: FnMut(Action, &str, &str, i64)
+    where
+        F: FnMut(Action, &str, &str, i64),
     {
         self.db.borrow_mut().update_hook(hook);
     }
-
-    /// Remove hook installed by `update_hook`.
-    pub fn remove_update_hook(&self) {
-        self.db.borrow_mut().remove_update_hook();
-    }
-
-    /// Remove hook installed by `commit_hook`.
-    pub fn remove_commit_hook(&self) {
-        self.db.borrow_mut().remove_commit_hook();
-    }
-
-    /// Remove hook installed by `rollback_hook`.
-    pub fn remove_rollback_hook(&self) {
-        self.db.borrow_mut().remove_rollback_hook();
-    }
 }
 
 impl InnerConnection {
     pub fn remove_hooks(&mut self) {
-        self.remove_update_hook();
+        self.update_hook(None::<fn(Action, &str, &str, i64)>);
-        self.remove_commit_hook();
+        self.commit_hook(None::<fn() -> bool>);
-        self.remove_rollback_hook();
+        self.rollback_hook(None::<fn()>);
     }
 
-    fn commit_hook<F>(&self, hook: F)
+    fn commit_hook<F>(&mut self, hook: Option<F>)
-        where F: FnMut() -> bool
+    where
+        F: FnMut() -> bool,
     {
         unsafe extern "C" fn call_boxed_closure<F>(p_arg: *mut c_void) -> c_int
-            where F: FnMut() -> bool
+        where
+            F: FnMut() -> bool,
         {
             let boxed_hook: *mut F = p_arg as *mut F;
-            assert!(!boxed_hook.is_null(),
+            if (*boxed_hook)() {
-                    "Internal error - null function pointer");
+                1
-
+            } else {
-            if (*boxed_hook)() { 1 } else { 0 }
+                0
+            }
         }
 
-        let previous_hook = {
+        // unlike `sqlite3_create_function_v2`, we cannot specify a `xDestroy` with `sqlite3_commit_hook`.
-            let boxed_hook: *mut F = Box::into_raw(Box::new(hook));
+        // so we keep the `xDestroy` function in `InnerConnection.free_boxed_hook`.
-            unsafe {
+        let free_commit_hook = if hook.is_some() {
-                ffi::sqlite3_commit_hook(self.db(),
+            Some(free_boxed_hook::<F> as fn(*mut c_void))
-                                         Some(call_boxed_closure::<F>),
+        } else {
-                                         boxed_hook as *mut _)
+            None
-            }
         };
-        free_boxed_hook(previous_hook);
+
+        let previous_hook = match hook {
+            Some(hook) => {
+                let boxed_hook: *mut F = Box::into_raw(Box::new(hook));
+                unsafe {
+                    ffi::sqlite3_commit_hook(
+                        self.db(),
+                        Some(call_boxed_closure::<F>),
+                        boxed_hook as *mut _,
+                    )
+                }
+            }
+            _ => unsafe { ffi::sqlite3_commit_hook(self.db(), None, ptr::null_mut()) },
+        };
+        if !previous_hook.is_null() {
+            if let Some(free_boxed_hook) = self.free_commit_hook {
+                free_boxed_hook(previous_hook);
+            }
+        }
+        self.free_commit_hook = free_commit_hook;
     }
 
-    fn rollback_hook<F>(&self, hook: F)
+    fn rollback_hook<F>(&mut self, hook: Option<F>)
-        where F: FnMut()
+    where
+        F: FnMut(),
     {
         unsafe extern "C" fn call_boxed_closure<F>(p_arg: *mut c_void)
-            where F: FnMut()
+        where
+            F: FnMut(),
         {
             let boxed_hook: *mut F = p_arg as *mut F;
-            assert!(!boxed_hook.is_null(),
-                    "Internal error - null function pointer");
-
             (*boxed_hook)();
         }
 
-        let previous_hook = {
+        let free_rollback_hook = if hook.is_some() {
-            let boxed_hook: *mut F = Box::into_raw(Box::new(hook));
+            Some(free_boxed_hook::<F> as fn(*mut c_void))
-            unsafe {
+        } else {
-                ffi::sqlite3_rollback_hook(self.db(),
+            None
-                                           Some(call_boxed_closure::<F>),
-                                           boxed_hook as *mut _)
-            }
         };
-        free_boxed_hook(previous_hook);
+
+        let previous_hook = match hook {
+            Some(hook) => {
+                let boxed_hook: *mut F = Box::into_raw(Box::new(hook));
+                unsafe {
+                    ffi::sqlite3_rollback_hook(
+                        self.db(),
+                        Some(call_boxed_closure::<F>),
+                        boxed_hook as *mut _,
+                    )
+                }
+            }
+            _ => unsafe { ffi::sqlite3_rollback_hook(self.db(), None, ptr::null_mut()) },
+        };
+        if !previous_hook.is_null() {
+            if let Some(free_boxed_hook) = self.free_rollback_hook {
+                free_boxed_hook(previous_hook);
+            }
+        }
+        self.free_rollback_hook = free_rollback_hook;
     }
 
-    fn update_hook<F>(&mut self, hook: F)
+    fn update_hook<F>(&mut self, hook: Option<F>)
-        where F: FnMut(Action, &str, &str, i64)
+    where
+        F: FnMut(Action, &str, &str, i64),
     {
-        unsafe extern "C" fn call_boxed_closure<F>(p_arg: *mut c_void,
+        unsafe extern "C" fn call_boxed_closure<F>(
-                                                   action_code: c_int,
+            p_arg: *mut c_void,
-                                                   db_str: *const c_char,
+            action_code: c_int,
-                                                   tbl_str: *const c_char,
+            db_str: *const c_char,
-                                                   row_id: i64)
+            tbl_str: *const c_char,
-            where F: FnMut(Action, &str, &str, i64)
+            row_id: i64,
+        ) where
+            F: FnMut(Action, &str, &str, i64),
         {
             use std::ffi::CStr;
             use std::str;
 
             let boxed_hook: *mut F = p_arg as *mut F;
-            assert!(!boxed_hook.is_null(),
-                    "Internal error - null function pointer");
 
             let action = Action::from(action_code);
             let db_name = {
@@ -225,38 +250,36 @@ impl InnerConnection {
             (*boxed_hook)(action, db_name, tbl_name, row_id);
         }
 
-        let previous_hook = {
+        let free_update_hook = if hook.is_some() {
-            let boxed_hook: *mut F = Box::into_raw(Box::new(hook));
+            Some(free_boxed_hook::<F> as fn(*mut c_void))
-            unsafe {
+        } else {
-                ffi::sqlite3_update_hook(self.db(),
+            None
-                                         Some(call_boxed_closure::<F>),
-                                         boxed_hook as *mut _)
-            }
         };
-        free_boxed_hook(previous_hook);
-    }
 
-    fn remove_update_hook(&mut self) {
+        let previous_hook = match hook {
-        let previous_hook = unsafe { ffi::sqlite3_update_hook(self.db(), None, ptr::null_mut()) };
+            Some(hook) => {
-        free_boxed_hook(previous_hook);
+                let boxed_hook: *mut F = Box::into_raw(Box::new(hook));
-    }
+                unsafe {
-
+                    ffi::sqlite3_update_hook(
-    fn remove_commit_hook(&mut self) {
+                        self.db(),
-        let previous_hook = unsafe { ffi::sqlite3_commit_hook(self.db(), None, ptr::null_mut()) };
+                        Some(call_boxed_closure::<F>),
-        free_boxed_hook(previous_hook);
+                        boxed_hook as *mut _,
-    }
+                    )
-
+                }
-    fn remove_rollback_hook(&mut self) {
+            }
-        let previous_hook = unsafe { ffi::sqlite3_rollback_hook(self.db(), None, ptr::null_mut()) };
+            _ => unsafe { ffi::sqlite3_update_hook(self.db(), None, ptr::null_mut()) },
-        free_boxed_hook(previous_hook);
+        };
+        if !previous_hook.is_null() {
+            if let Some(free_boxed_hook) = self.free_update_hook {
+                free_boxed_hook(previous_hook);
+            }
+        }
+        self.free_update_hook = free_update_hook;
     }
 }
 
-fn free_boxed_hook(hook: *mut c_void) {
+fn free_boxed_hook<F>(p: *mut c_void) {
-    if !hook.is_null() {
+    drop(unsafe { Box::from_raw(p as *mut F) });
-        // TODO make sure that size_of::<*mut F>() is always equal to size_of::<*mut c_void>()
-        let _: Box<*mut c_void> = unsafe { Box::from_raw(hook as *mut _) };
-    }
 }
 
 #[cfg(test)]
@@ -269,21 +292,36 @@ mod test {
         let db = Connection::open_in_memory().unwrap();
 
         let mut called = false;
-        db.commit_hook(|| {
+        db.commit_hook(Some(|| {
-                           called = true;
+            called = true;
-                           false
+            false
-                       });
+        }));
         db.execute_batch("BEGIN; CREATE TABLE foo (t TEXT); COMMIT;")
             .unwrap();
         assert!(called);
     }
 
+    #[test]
+    fn test_fn_commit_hook() {
+        let db = Connection::open_in_memory().unwrap();
+
+        fn hook() -> bool {
+            true
+        }
+
+        db.commit_hook(Some(hook));
+        db.execute_batch("BEGIN; CREATE TABLE foo (t TEXT); COMMIT;")
+            .unwrap_err();
+    }
+
     #[test]
     fn test_rollback_hook() {
         let db = Connection::open_in_memory().unwrap();
 
         let mut called = false;
-        db.rollback_hook(|| { called = true; });
+        db.rollback_hook(Some(|| {
+            called = true;
+        }));
         db.execute_batch("BEGIN; CREATE TABLE foo (t TEXT); ROLLBACK;")
             .unwrap();
         assert!(called);
@@ -294,13 +332,13 @@ mod test {
         let db = Connection::open_in_memory().unwrap();
 
         let mut called = false;
-        db.update_hook(|action, db, tbl, row_id| {
+        db.update_hook(Some(|action, db: &str, tbl: &str, row_id| {
-                           assert_eq!(Action::SQLITE_INSERT, action);
+            assert_eq!(Action::SQLITE_INSERT, action);
-                           assert_eq!("main", db);
+            assert_eq!("main", db);
-                           assert_eq!("foo", tbl);
+            assert_eq!("foo", tbl);
-                           assert_eq!(1, row_id);
+            assert_eq!(1, row_id);
-                           called = true;
+            called = true;
-                       });
+        }));
         db.execute_batch("CREATE TABLE foo (t TEXT)").unwrap();
         db.execute_batch("INSERT INTO foo VALUES ('lisa')").unwrap();
         assert!(called);

src/lib.rs

@@ -72,7 +72,7 @@ use std::result;
 use std::str;
 use std::sync::{Once, ONCE_INIT};
 use std::sync::atomic::{AtomicBool, ATOMIC_BOOL_INIT, Ordering};
-use std::os::raw::{c_int, c_char};
+use std::os::raw::{c_int, c_char, c_void};
 
 use types::{ToSql, ValueRef};
 use error::{error_from_sqlite_code, error_from_handle};
@@ -597,6 +597,12 @@ impl fmt::Debug for Connection {
 
 struct InnerConnection {
     db: *mut ffi::sqlite3,
+    #[cfg(feature = "hooks")]
+    free_commit_hook: Option<fn(*mut c_void)>,
+    #[cfg(feature = "hooks")]
+    free_rollback_hook: Option<fn(*mut c_void)>,
+    #[cfg(feature = "hooks")]
+    free_update_hook: Option<fn(*mut c_void)>,
 }
 
 /// Old name for `OpenFlags`. `SqliteOpenFlags` is deprecated.
@@ -755,6 +761,15 @@ To fix this, either:
 }
 
 impl InnerConnection {
+    #[cfg(not(feature = "hooks"))]
+    fn new(db: *mut ffi::sqlite3) -> InnerConnection {
+        InnerConnection { db }
+    }
+    #[cfg(feature = "hooks")]
+    fn new(db: *mut ffi::sqlite3) -> InnerConnection {
+        InnerConnection { db, free_commit_hook: None, free_rollback_hook: None, free_update_hook: None }
+    }
+
     fn open_with_flags(c_path: &CString, flags: OpenFlags) -> Result<InnerConnection> {
         ensure_valid_sqlite_version();
         ensure_safe_sqlite_threading_mode()?;
@@ -792,7 +807,7 @@ impl InnerConnection {
             // attempt to turn on extended results code; don't fail if we can't.
             ffi::sqlite3_extended_result_codes(db, 1);
 
-            Ok(InnerConnection { db })
+            Ok(InnerConnection::new(db))
         }
     }