duckdb / duckdb-rs

Ergonomic bindings to duckdb for Rust
MIT License
511 stars 113 forks source link

impl Drop for Appender has unnecessary unsafe calls. #266

Open era127 opened 9 months ago

era127 commented 9 months ago

According to the C appender api documentation, the drop of appender should only need to call duckdb_appender_destroy. I believe the flush and duckdb_appender_close call can be removed.

impl Drop for Appender<'_> {
    fn drop(&mut self) {
        if !self.app.is_null() {
  // remove     self.flush();
            unsafe {
  // remove     ffi::duckdb_appender_close(self.app);
                ffi::duckdb_appender_destroy(&mut self.app);
            }
        }
    }
}
Swoorup commented 5 months ago

imo, it's better to wrap the appender handle into its own type, and use that instead like for DataChunkHandle here. There is few other places where the appender is passed around, which could possibly cause use after free errors.