Skip to content
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

Fix python SDK's shutdown unsafely dropping cross-FFI resources #8038

Merged
merged 1 commit into from
Nov 7, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion rerun_py/src/python_bridge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,9 +314,21 @@ fn shutdown(py: Python<'_>) {
re_log::debug!("Shutting down the Rerun SDK");
// Release the GIL in case any flushing behavior needs to cleanup a python object.
py.allow_threads(|| {
for (_, recording) in all_recordings().drain() {
// NOTE: Do **NOT** try and drain() `all_recordings` here.
//
// Doing so would drop the last remaining reference to these recordings, and therefore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for nicely clarifying, this is indeed nuanced

// trigger their deallocation as well as the deallocation of all the Python and C++ data
// that they might transitively reference, but this is _NOT_ the right place to do so.
// This method is called automatically during shutdown via python's `atexit`, which is not
// a safepoint for deallocating these things, quite far from it.
//
// Calling `disconnect()` will already take care of flushing everything that can be flushed,
// and cleaning up everything that can be safely cleaned up, anyhow.
// Whatever's left can wait for the OS to clean it up.
for (_, recording) in all_recordings().iter() {
recording.disconnect();
}

flush_garbage_queue();
});
}
Expand Down
Loading