Skip to content

Commit

Permalink
Fix potential deadlock when saving to file after logging at the end o…
Browse files Browse the repository at this point in the history
…f a Python program (#3920)

### What
Certain edge cases of changing the sink can cause an internal flush,
which is known to cause potential deadlocks when dropping python
datastructures.

Release the GIL under .save(), .connect(), and .memory().

Resolves:
 - #3907

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/3920) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/3920)
- [Docs
preview](https://rerun.io/preview/c5bcfffb3612a6ede8c94a8d05894155e56bbe15/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/c5bcfffb3612a6ede8c94a8d05894155e56bbe15/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
  • Loading branch information
jleibs authored Oct 19, 2023
1 parent 22e0188 commit 2f4cfe3
Showing 1 changed file with 31 additions and 17 deletions.
48 changes: 31 additions & 17 deletions rerun_py/src/python_bridge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,7 @@ fn connect(
addr: Option<String>,
flush_timeout_sec: Option<f32>,
recording: Option<&PyRecordingStream>,
py: Python<'_>,
) -> PyResult<()> {
let addr = if let Some(addr) = addr {
addr.parse()?
Expand All @@ -480,40 +481,53 @@ fn connect(

let flush_timeout = flush_timeout_sec.map(std::time::Duration::from_secs_f32);

if let Some(recording) = recording {
// If the user passed in a recording, use it
recording.connect(addr, flush_timeout);
} else {
// Otherwise, connect both global defaults
if let Some(recording) = get_data_recording(None) {
// The call to connect may internally flush.
// Release the GIL in case any flushing behavior needs to cleanup a python object.
py.allow_threads(|| {
if let Some(recording) = recording {
// If the user passed in a recording, use it
recording.connect(addr, flush_timeout);
};
if let Some(blueprint) = get_blueprint_recording(None) {
blueprint.connect(addr, flush_timeout);
};
}
} else {
// Otherwise, connect both global defaults
if let Some(recording) = get_data_recording(None) {
recording.connect(addr, flush_timeout);
};
if let Some(blueprint) = get_blueprint_recording(None) {
blueprint.connect(addr, flush_timeout);
};
}
});

Ok(())
}

#[pyfunction]
#[pyo3(signature = (path, recording = None))]
fn save(path: &str, recording: Option<&PyRecordingStream>) -> PyResult<()> {
fn save(path: &str, recording: Option<&PyRecordingStream>, py: Python<'_>) -> PyResult<()> {
let Some(recording) = get_data_recording(recording) else {
return Ok(());
};

recording
.save(path)
.map_err(|err| PyRuntimeError::new_err(err.to_string()))
// The call to save may internally flush.
// Release the GIL in case any flushing behavior needs to cleanup a python object.
py.allow_threads(|| {
recording
.save(path)
.map_err(|err| PyRuntimeError::new_err(err.to_string()))
})
}

/// Create an in-memory rrd file
#[pyfunction]
#[pyo3(signature = (recording = None))]
fn memory_recording(recording: Option<&PyRecordingStream>) -> Option<PyMemorySinkStorage> {
fn memory_recording(
recording: Option<&PyRecordingStream>,
py: Python<'_>,
) -> Option<PyMemorySinkStorage> {
get_data_recording(recording).map(|rec| {
let inner = rec.memory();
// The call to memory may internally flush.
// Release the GIL in case any flushing behavior needs to cleanup a python object.
let inner = py.allow_threads(|| rec.memory());
PyMemorySinkStorage { rec: rec.0, inner }
})
}
Expand Down

1 comment on commit 2f4cfe3

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.25.

Benchmark suite Current: 2f4cfe3 Previous: 22e0188 Ratio
mono_points_arrow/generate_message_bundles 26506138 ns/iter (± 1240148) 18546498 ns/iter (± 243845) 1.43
mono_points_arrow_batched/generate_message_bundles 18325594 ns/iter (± 1426671) 14245082 ns/iter (± 23142) 1.29
mono_points_arrow_batched/encode_total 24962822 ns/iter (± 1516326) 19661293 ns/iter (± 86027) 1.27

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.