Repository: cassandra-rs/cassandra-rs (language: rust)

Issue:
Every _CassResult is leaked by CassResult
The Drop trait for CassResult was commented out to work around an issue with bind_by_name apparently causing double-frees.

See [the commented out code here](https://github.com/Metaswitch/cassandra-rs/blob/58ab265e4bcd0831ebfb6d897938881bf391d0b6/src/cassandra/result.rs#L70-L75)


Discussion / hints:
Thanks @CorbinH - unfortunately there are a number of leaks like this. We intend to beef up the test suite and start addressing them at some point, but at the moment we're focussing on fixing functional and API issues. We'd happily accept a PR if you want to have a go - see [CONTRIBUTING.md](https://github.com/Metaswitch/cassandra-rs/blob/master/CONTRIBUTING.md).

The existing `bind_by_name` tests are in https://github.com/Metaswitch/cassandra-rs/blob/master/tests/bind_by_name.rs but you might need to extend them to be confident the issues are properly fixed. I'm happy to review your code and help you diagnose any issues.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 26ebbd6..e0d5df6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,97 +1,109 @@
-## 0.11.1-pre (unreleased)
+# Changelog
 
-* (tbd)
+All notable changes to this project will be documented in this file.
 
-## 0.11.0 (2018-04-26)
+This file's format is based on [Keep a Changelog](http://keepachangelog.com/)
+and this project adheres to [Semantic Versioning](http://semver.org/). The
+version number is tracked in the file `VERSION`.
 
-* Remove the `AsInet` and `FromInet` traits, replacing them with suitable implementations of `From`.
-* Fixed buggy IPv6 conversions.
-* `Inet::cass_inet_init_v4` and `Inet::cass_inet_init_v6` no longer consume their arguments.
-* `Tuple::set_inet()` now takes an `IpAddr` rather than a `SocketAddr`.
-* Added wrapper for `cass_statement_set_request_timeout`.
+## [Unreleased]
+### Fixed
+- No longer leaks all `CassResult`s.
+### Changed
+- `ResultIterator` now has a lifetime parameter. The underlying `CassResult` must live for at
+  least as long as the iterator.
+- `CassResult` is no longer `IntoIterator`; instead `&CassResult` is. You must change code
+  like `for row in result` to `for row in &result` and ensure `result` lives long enough.
 
-## 0.10.2 (2017-09-11)
+## [0.11.0] - 2018-04-26
+- Remove the `AsInet` and `FromInet` traits, replacing them with suitable implementations of `From`.
+- Fixed buggy IPv6 conversions.
+- `Inet::cass_inet_init_v4` and `Inet::cass_inet_init_v6` no longer consume their arguments.
+- `Tuple::set_inet()` now takes an `IpAddr` rather than a `SocketAddr`.
+- Added wrapper for `cass_statement_set_request_timeout`.
 
-* Move to latest (0.11) version of `cassandra-cpp-sys` crate.
-  There should be no external impact.
-
-
-## 0.10.1 (2017-08-30)
-
-* Remove unnecessary dependency on `ip` crate.
-* Add `Copy`, `Clone`, `Hash` impls for all nullary enums.
-* Specify correct `Send` and `Sync` markers for all C* types.
+## [0.10.2] - 2017-09-11
+- Move to latest (0.11) version of `cassandra-cpp-sys` crate.
 
+  There should be no external impact.
 
-## 0.10.0 (2017-08-03)
+## [0.10.1] - 2017-08-30
+- Remove unnecessary dependency on `ip` crate.
+- Add `Copy`, `Clone`, `Hash` impls for all nullary enums.
+- Specify correct `Send` and `Sync` markers for all C* types.
 
+## [0.10.0] - 2017-08-03
 The API changed significantly in version 0.10. Here is a summary of the main changes.
 (Version 0.9 was skipped, for consistency with the `cassandra-cpp-sys` version number.)
 
 Errors:
 
-* The internal module `errors` and the underlying `cassandra-cpp-sys` crate are
+- The internal module `errors` and the underlying `cassandra-cpp-sys` crate are
   no longer exposed in the API.
   All necessary types are defined in this crate's root module.
-* All errors are now reported consistently using a single newly-defined `Error` type.
-  * The crate makes every effort to return an error rather than panicking.
-  * `CassError`, `CassErrorResult`, and others are replaced by `Error` and
+- All errors are now reported consistently using a single newly-defined `Error` type.
+  - The crate makes every effort to return an error rather than panicking.
+  - `CassError`, `CassErrorResult`, and others are replaced by `Error` and
   `CassErrorCode`.
-  * Several return types have changed from `T` to `Result<T, Error>`.
+  - Several return types have changed from `T` to `Result<T, Error>`.
 
 Futures:
 
-* There is only a single future type, `CassFuture`, and it implements the
+- There is only a single future type, `CassFuture`, and it implements the
   Rust/Tokio [futures](https://docs.rs/futures) API. It interoperates smoothly
   with existing futures code.
-  * `Future`, `CloseFuture`, `ResultFuture`, `PreparedFuture`, `SessionFuture`
+  - `Future`, `CloseFuture`, `ResultFuture`, `PreparedFuture`, `SessionFuture`
     are all subsumed.
-  * `wait` is replaced with `Future::wait`; other methods have standard analogues
+  - `wait` is replaced with `Future::wait`; other methods have standard analogues
     as well. See the [futures]((https://docs.rs/futures)) documentation for details.
-  * Callbacks can no longer be set explicitly on a future; instead the normal
+  - Callbacks can no longer be set explicitly on a future; instead the normal
     futures mechanisms (e.g., `and_then`) should be used.
 
 Values:
 
-* The `Column` type is retired; instead use `Value`.
-* Some `Value` getters have new names for consistency, e.g.,
+- The `Column` type is retired; instead use `Value`.
+- Some `Value` getters have new names for consistency, e.g.,
   `get_flt` and `get_dbl` are now `get_f32` and `get_f64` respectively.
-* `Value::get_string` now gets a `String`, not a `&str`; you can get a `&str` with `get_str`.
-* The "magic" auto-converting `Row` getters `get_col` and `get_col_by_name` are renamed
+- `Value::get_string` now gets a `String`, not a `&str`; you can get a `&str` with `get_str`.
+- The "magic" auto-converting `Row` getters `get_col` and `get_col_by_name` are renamed
   to `get` and `get_by_name` respectively. This is to avoid confusion with `get_column`, which is
   something else entirely (it gets a `Value` from a `Row`).
-* Values have a new `is_null` method to allow retrieving null values.
-* UUIDs now support `Eq` and `Ord`.
+- Values have a new `is_null` method to allow retrieving null values.
+- UUIDs now support `Eq` and `Ord`.
 
 Miscellaneous types:
 
-* Several types which wrapped `cassandra-cpp-sys` types now have enums of their
+- Several types which wrapped `cassandra-cpp-sys` types now have enums of their
   own, complete with implementations of `Debug`, `Eq`, `PartialEq`, `Display`,
   and `FromStr`. This includes `BatchType`, `CassErrorCode`, `Consistency`,
   `LogLevel`, `SslVerifyFlag`, and `ValueType`.
-* `CqlProtocol` is now simply an alias for an integer.
-* Contact points are now expressed as a simple string in the driver's preferred format.
+- `CqlProtocol` is now simply an alias for an integer.
+- Contact points are now expressed as a simple string in the driver's preferred format.
   `ContactPoints` is retired.
 
 Other:
 
-* Logging uses the `slog` crate. It is no longer possible to set your own logging
+- Logging uses the `slog` crate. It is no longer possible to set your own logging
   callback, but you can set the `slog` logger.
-* Internally, the code is cleaner and smaller and some tests have been added.
-
-
-## 0.8.2 (2017-06-29)
+- Internally, the code is cleaner and smaller and some tests have been added.
 
+## [0.8.2] - 2017-06-29
 First release of https://github.com/Metaswitch/cassandra-rs
 
-* Fork package.
-* Move examples to examples directory, then make several into Rust tests.
-* Resolve all warnings.
-* Fix various panics.
-* Add `Eq` and `Ord` for `Uuid`.
-* Add `is_null` support.
-
-
-## 0.8.1 (2016-12-13)
+- Fork package.
+- Move examples to examples directory, then make several into Rust tests.
+- Resolve all warnings.
+- Fix various panics.
+- Add `Eq` and `Ord` for `Uuid`.
+- Add `is_null` support.
 
+## [0.8.1] - 2016-12-13
 Last release of https://github.com/tupshin/cassandra-rs
+
+[Unreleased]: https://github.com/Metaswitch/cassandra-rs/compare/0.11.0...HEAD
+[0.11.0]: https://github.com/Metaswitch/cassandra-rs/compare/0.10.2...0.11.0
+[0.10.2]: https://github.com/Metaswitch/cassandra-rs/compare/0.10.1...0.10.2
+[0.10.1]: https://github.com/Metaswitch/cassandra-rs/compare/0.10.0...0.10.1
+[0.10.0]: https://github.com/Metaswitch/cassandra-rs/compare/0.8.2...0.10.0
+[0.8.2]: https://github.com/Metaswitch/cassandra-rs/compare/0.8.1...0.8.2
+[0.8.1]: https://github.com/Metaswitch/cassandra-rs/tree/0.8.1
diff --git a/src/cassandra/result.rs b/src/cassandra/result.rs
index c7c9516..bce5063 100644
--- a/src/cassandra/result.rs
+++ b/src/cassandra/result.rs
@@ -31,6 +31,7 @@ use std::fmt;
 use std::fmt::Debug;
 use std::fmt::Display;
 use std::fmt::Formatter;
+use std::marker::PhantomData;
 use std::mem;
 use std::slice;
 use std::str;
@@ -67,12 +68,11 @@ impl Display for CassResult {
     }
 }
 
-// FIXME Understand why this drop results in double freeing when binding by name
-// impl Drop for CassResult {
-//    fn drop(&mut self) {
-//        unsafe { cass_result_free(self.0) }
-//    }
-// }
+ impl Drop for CassResult {
+    fn drop(&mut self) {
+        unsafe { cass_result_free(self.0) }
+    }
+ }
 
 impl CassResult {
     /// Gets the number of rows for the specified result.
@@ -135,22 +135,23 @@ impl CassResult {
 
     /// Creates a new iterator for the specified result. This can be
     /// used to iterate over rows in the result.
-    pub fn iter(&self) -> ResultIterator { unsafe { ResultIterator(cass_iterator_from_result(self.0)) } }
+    pub fn iter(&self) -> ResultIterator { unsafe { ResultIterator(cass_iterator_from_result(self.0), PhantomData) } }
 }
 
-/// An iterator over the results of a query
+/// An iterator over the results of a query.
+/// The result holds the data, so it must last for at least the lifetime of the iterator.
 #[derive(Debug)]
-pub struct ResultIterator(pub *mut _CassIterator);
+pub struct ResultIterator<'a>(pub *mut _CassIterator, PhantomData<&'a CassResult>);
 
 // The underlying C type has no thread-local state, but does not support access
 // from multiple threads: https://datastax.github.io/cpp-driver/topics/#thread-safety
-unsafe impl Send for ResultIterator {}
+unsafe impl<'a> Send for ResultIterator<'a> {}
 
-impl Drop for ResultIterator {
+impl<'a> Drop for ResultIterator<'a> {
     fn drop(&mut self) { unsafe { cass_iterator_free(self.0) } }
 }
 
-impl Iterator for ResultIterator {
+impl<'a> Iterator for ResultIterator<'a> {
     type Item = Row;
     fn next(&mut self) -> Option<<Self as Iterator>::Item> {
         unsafe {
@@ -162,23 +163,14 @@ impl Iterator for ResultIterator {
     }
 }
 
-impl ResultIterator {
+impl<'a> ResultIterator<'a> {
     /// Gets the next row in the result set
     pub fn get_row(&mut self) -> Row { unsafe { Row::build(cass_iterator_get_row(self.0)) } }
 }
 
-impl IntoIterator for CassResult {
+impl<'a> IntoIterator for &'a CassResult {
     type Item = Row;
-    type IntoIter = ResultIterator;
+    type IntoIter = ResultIterator<'a>;
 
     fn into_iter(self) -> Self::IntoIter { self.iter() }
 }
-
-// impl<'a> IntoIterator for &'a CassandraResult {
-//    type Item = Row;
-//    type IntoIter = ResultIterator;
-//
-//    fn into_iter(self) -> Self::IntoIter {unsafe{
-//        ResultIterator(cass_iterator_from_result(self.0))
-//    }}
-// }


Test patch:
diff --git a/tests/batch.rs b/tests/batch.rs
index b19d4ef..14053ad 100644
--- a/tests/batch.rs
+++ b/tests/batch.rs
@@ -37,12 +37,13 @@ fn retrieve_batch(session: &Session) -> Vec<Pair> {
     let select_query = stmt!("SELECT * from examples.pairs");
 
     let result = session.execute(&select_query).wait().unwrap();
-    result.iter().map(|r| {
+    let v: Vec<Pair> = result.iter().map(|r| {
         Pair {
             key: r.get(0).expect("Key"),
             value: r.get(1).expect("Value"),
         }
-    }).collect()
+    }).collect();
+    v
 }
 
 #[test]
diff --git a/tests/bind_by_name.rs b/tests/bind_by_name.rs
index 211e840..27fafc7 100644
--- a/tests/bind_by_name.rs
+++ b/tests/bind_by_name.rs
@@ -22,7 +22,7 @@ fn test_bind_by_name() {
     let schema_query = Statement::new(&query, 0);
     for _ in 0..1000 {
         let result = session.execute(&schema_query).wait().unwrap();
-        for row in result {
+        for row in &result {
             let name: String = row.get_by_name("column_name").unwrap();
             let ftype: String = row.get_by_name("type").unwrap();
             // Actual values are not important; we're checking it doesn't panic or fail to return info.