Skip to content

Commit

Permalink
Document constraints of Viewer's continuous read behavior, fix relate…
Browse files Browse the repository at this point in the history
…d test (#7517)

### What

Our ability to load files as they are written is limited by both
flushing & notification behavior of OS & filesystem.
This is now documented on the `save` methods.

See also
* notify-rs/notify#422

Also, fixes a test that failed because of this on Windows (nightly ci).


### 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 the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/7517?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/7517?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/7517)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
  • Loading branch information
Wumpf authored Sep 26, 2024
1 parent 6976742 commit b13ace0
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 6 deletions.
39 changes: 33 additions & 6 deletions crates/store/re_data_loader/src/loader_rrd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,14 +219,32 @@ mod tests {
use re_log_types::{
ApplicationId, LogMsg, SetStoreInfo, StoreId, StoreInfo, StoreKind, StoreSource, Time,
};
use tempfile::NamedTempFile;

use super::*;

struct DeleteOnDrop {
path: std::path::PathBuf,
}

impl Drop for DeleteOnDrop {
fn drop(&mut self) {
std::fs::remove_file(&self.path).ok();
}
}

#[test]
fn test_loading_with_retryable_reader() {
let rrd_file = NamedTempFile::new().unwrap();
let rrd_file_path = rrd_file.path().to_owned();
// We can't use `tempfile` here since it deletes the file on drop and we want to keep it around for a bit longer.
let rrd_file_path = std::path::PathBuf::from("testfile.rrd");
let rrd_file_delete_guard = DeleteOnDrop {
path: rrd_file_path.clone(),
};
std::fs::remove_file(&rrd_file_path).ok(); // Remove the file just in case a previous test crashes hard.
let rrd_file = std::fs::OpenOptions::new()
.create(true)
.write(true)
.open(rrd_file_path.to_str().unwrap())
.unwrap();

let mut encoder = Encoder::new(
re_build_info::CrateVersion::LOCAL,
Expand Down Expand Up @@ -258,6 +276,7 @@ mod tests {
for m in &messages {
encoder.append(m).expect("failed to append message");
}
encoder.flush_blocking().expect("failed to flush messages");

let reader = RetryableFileReader::new(&rrd_file_path).unwrap();
let mut decoder = Decoder::new(decoder::VersionPolicy::Warn, reader).unwrap();
Expand Down Expand Up @@ -287,11 +306,19 @@ mod tests {
for m in &more_messages {
encoder.append(m).unwrap();
}
// close the stream to stop the decoder reading, otherwise with retryable reader we'd be waiting indefinitely
encoder.finish().unwrap();
// Close the encoder and thus the file to make sure that file is actually written out.
// Otherwise we can't we be sure that the filewatcher will ever see those changes.
// A simple flush works sometimes, but is not as reliably as closing the file since the OS may still cache the data.
// (in fact we can't be sure that close is enough either, but it's the best we can do)
// Note that this test is not entirely representative of the real usecase of having reader and writer on
// different processes, since file read/write visibility across processes may behave differently.
encoder.finish().expect("failed to finish encoder");
drop(encoder);

let remaining_messages = decoder_handle.join().unwrap();

assert_eq!(more_messages, remaining_messages);

// Drop explicitly to make sure that rustc doesn't drop it earlier.
drop(rrd_file_delete_guard);
}
}
4 changes: 4 additions & 0 deletions crates/top/re_sdk/src/recording_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,10 @@ impl RecordingStreamBuilder {
/// Creates a new [`RecordingStream`] that is pre-configured to stream the data through to an
/// RRD file on disk.
///
/// The Rerun Viewer is able to read continuously from the resulting rrd file while it is being written.
/// However, depending on your OS and configuration, changes may not be immediately visible due to file caching.
/// This is a common issue on Windows and (to a lesser extent) on MacOS.
///
/// ## Example
///
/// ```no_run
Expand Down
4 changes: 4 additions & 0 deletions rerun_cpp/src/rerun/recording_stream.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ namespace rerun {

/// Stream all log-data to a given `.rrd` file.
///
/// The Rerun Viewer is able to read continuously from the resulting rrd file while it is being written.
/// However, depending on your OS and configuration, changes may not be immediately visible due to file caching.
/// This is a common issue on Windows and (to a lesser extent) on MacOS.
///
/// This function returns immediately.
Error save(std::string_view path) const;

Expand Down
4 changes: 4 additions & 0 deletions rerun_py/rerun_sdk/rerun/sinks.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ def save(
Call this _before_ you log any data!
The Rerun Viewer is able to read continuously from the resulting rrd file while it is being written.
However, depending on your OS and configuration, changes may not be immediately visible due to file caching.
This is a common issue on Windows and (to a lesser extent) on MacOS.
Parameters
----------
path:
Expand Down

0 comments on commit b13ace0

Please sign in to comment.