-
Notifications
You must be signed in to change notification settings - Fork 1.9k
FFI_RecordBatchStream was causing a memory leak #17190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FFI_RecordBatchStream was causing a memory leak #17190
Conversation
…mpletion. This is a temporary work around for apache/datafusion#17190
alamb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @timsaucer -- this makes sense to me
| } | ||
|
|
||
| unsafe extern "C" fn release_fn_wrapper(provider: &mut FFI_RecordBatchStream) { | ||
| let private_data = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked that the private_data is actually a RecordBatchStreamPrivateData that is created with the constructor
datafusion/datafusion/ffi/src/record_batch_stream.rs
Lines 80 to 84 in 327f8bc
| pub fn new(stream: SendableRecordBatchStream, runtime: Option<Handle>) -> Self { | |
| let private_data = Box::into_raw(Box::new(RecordBatchStreamPrivateData { | |
| rbs: stream, | |
| runtime, | |
| })) as *mut c_void; |
|
I also merged up from main |
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
Rationale for this change
When using the foreign function interface, a
Box<RecordBatchStreamPrivateData>is created but it is never properly freed. This leads to a growth in memory over time as these are used.What changes are included in this PR?
Adds a
releasefunction to FFI_RecordBatchStream and implementsDrop.Are these changes tested?
Tested against
datafusion-pythonwhere it is easy to track memory growth and to implement FFI based table providers.Are there any user-facing changes?
None