Skip to content

Commit

Permalink
Fix pyarrow memory leak (#3683)
Browse files Browse the repository at this point in the history
Remove ArrowArray::into_raw and try_from_raw
  • Loading branch information
tustvold committed Mar 21, 2023
1 parent 0df2188 commit 8573b38
Show file tree
Hide file tree
Showing 4 changed files with 7 additions and 65 deletions.
15 changes: 0 additions & 15 deletions arrow/src/array/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,21 +43,6 @@ impl TryFrom<ArrayData> for ffi::ArrowArray {
}
}

/// Creates a new array from two FFI pointers. Used to import arrays from the C Data Interface
/// # Safety
/// Assumes that these pointers represent valid C Data Interfaces, both in memory
/// representation and lifetime via the `release` mechanism.
#[deprecated(note = "Use ArrowArray::new")]
#[allow(deprecated)]
pub unsafe fn make_array_from_raw(
array: *const ffi::FFI_ArrowArray,
schema: *const ffi::FFI_ArrowSchema,
) -> Result<ArrayRef> {
let array = ffi::ArrowArray::try_from_raw(array, schema)?;
let data = ArrayData::try_from(array)?;
Ok(make_array(data))
}

/// Exports an array to raw pointers of the C Data Interface provided by the consumer.
/// # Safety
/// Assumes that these pointers represent valid C Data Interfaces, both in memory
Expand Down
3 changes: 1 addition & 2 deletions arrow/src/array/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ pub use arrow_data::{
pub use arrow_data::transform::{Capacities, MutableArrayData};

#[cfg(feature = "ffi")]
#[allow(deprecated)]
pub use self::ffi::{export_array_into_raw, make_array_from_raw};
pub use self::ffi::export_array_into_raw;

// --------------------- Array's values comparison ---------------------

Expand Down
46 changes: 2 additions & 44 deletions arrow/src/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -394,14 +394,10 @@ pub trait ArrowArrayRef {
/// Its main responsibility is to expose functionality that requires
/// both [FFI_ArrowArray] and [FFI_ArrowSchema].
///
/// This struct has two main paths:
///
/// ## Import from the C Data Interface
/// * [ArrowArray::empty] to allocate memory to be filled by an external call
/// * [ArrowArray::try_from_raw] to consume two non-null allocated pointers
/// * [ArrowArray::new] to create an array from [`FFI_ArrowArray`] and [`FFI_ArrowSchema`]
/// ## Export to the C Data Interface
/// * [ArrowArray::try_new] to create a new [ArrowArray] from Rust-specific information
/// * [ArrowArray::into_raw] to expose two pointers for [FFI_ArrowArray] and [FFI_ArrowSchema].
/// * Use [`FFI_ArrowArray`] and [`FFI_ArrowSchema`] directly
///
/// # Safety
/// Whoever creates this struct is responsible for releasing their resources. Specifically,
Expand Down Expand Up @@ -480,38 +476,6 @@ impl ArrowArray {
Ok(ArrowArray { array, schema })
}

/// creates a new [ArrowArray] from two pointers. Used to import from the C Data Interface.
/// # Safety
/// See safety of [ArrowArray]
/// Note that this function will copy the content pointed by the raw pointers. Considering
/// the raw pointers can be from `Arc::into_raw` or other raw pointers, users must be responsible
/// on managing the allocation of the structs by themselves.
/// # Error
/// Errors if any of the pointers is null
#[deprecated(note = "Use ArrowArray::new")]
pub unsafe fn try_from_raw(
array: *const FFI_ArrowArray,
schema: *const FFI_ArrowSchema,
) -> Result<Self> {
if array.is_null() || schema.is_null() {
return Err(ArrowError::MemoryError(
"At least one of the pointers passed to `try_from_raw` is null"
.to_string(),
));
};

let array_mut = array as *mut FFI_ArrowArray;
let schema_mut = schema as *mut FFI_ArrowSchema;

let array_data = std::ptr::replace(array_mut, FFI_ArrowArray::empty());
let schema_data = std::ptr::replace(schema_mut, FFI_ArrowSchema::empty());

Ok(Self {
array: Arc::new(array_data),
schema: Arc::new(schema_data),
})
}

/// creates a new empty [ArrowArray]. Used to import from the C Data Interface.
/// # Safety
/// See safety of [ArrowArray]
Expand All @@ -520,12 +484,6 @@ impl ArrowArray {
let array = Arc::new(FFI_ArrowArray::empty());
ArrowArray { array, schema }
}

/// exports [ArrowArray] to the C Data Interface
#[deprecated(note = "Use FFI_ArrowArray and FFI_ArrowSchema directly")]
pub fn into_raw(this: ArrowArray) -> (*const FFI_ArrowArray, *const FFI_ArrowSchema) {
(Arc::into_raw(this.array), Arc::into_raw(this.schema))
}
}

#[cfg(test)]
Expand Down
8 changes: 4 additions & 4 deletions arrow/src/pyarrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,16 +133,16 @@ impl PyArrowConvert for ArrayData {
}

fn to_pyarrow(&self, py: Python) -> PyResult<PyObject> {
let array = ffi::ArrowArray::try_from(self.clone()).map_err(to_py_err)?;
let (array_pointer, schema_pointer) = ffi::ArrowArray::into_raw(array);
let array = FFI_ArrowArray::new(self);
let schema = FFI_ArrowSchema::try_from(self.data_type()).map_err(to_py_err)?;

let module = py.import("pyarrow")?;
let class = module.getattr("Array")?;
let array = class.call_method1(
"_import_from_c",
(
array_pointer as Py_uintptr_t,
schema_pointer as Py_uintptr_t,
addr_of_mut!(array) as Py_uintptr_t,
addr_of_mut!(schema) as Py_uintptr_t,
),
)?;
Ok(array.to_object(py))
Expand Down

0 comments on commit 8573b38

Please sign in to comment.