Skip to content

Commit

Permalink
Add MainThreadToken to ensure file-dialogs only run on the main thr…
Browse files Browse the repository at this point in the history
…ead (#8467)

### What

We have a foot-gun in our code: our file dialogs (via `rfd`) [are only
allowed to be run from the main thread (at least on
Mac)](https://docs.rs/rfd/latest/rfd/#macos-non-windowed-applications-async-and-threading).
However, there is nothing stopping a user from accidentally calling
these functions from a background thread, and if you test it on e.g.
Linux, it may very well work.

So this PR introduces a new crate `re_capabilities` and a new type
`MainThreadToken`. Any function that uses `rfd` should require the
`MainThreadToken` as an argument. The `MainThreadToken` is only allowed
to be created in `fn main`, and since it is neither `Send` nor `Sync`,
this guarantees at compile-time that any functions that take a
`MainThreadToken` can only be called from the main thread.

NOTE: I have no way to enforce that all uses of `rfd` also require
`MainThreadToken` - we need to remember this ourselves, but I've made
sure that all our _current_ uses of `rfd` require a `MainThreadToken`.
  • Loading branch information
emilk authored Dec 17, 2024
1 parent 4f9ba38 commit db18508
Show file tree
Hide file tree
Showing 30 changed files with 235 additions and 42 deletions.
1 change: 1 addition & 0 deletions ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ Update instructions:
| Crate | Description |
|--------------------|--------------------------------------------------------------------------------------|
| re_analytics | Rerun's analytics SDK |
| re_capabilities | Capability tokens |
| re_case | Case conversions, the way Rerun likes them |
| re_crash_handler | Detect panics and signals, logging them and optionally sending them to analytics. |
| re_error | Helpers for handling errors. |
Expand Down
13 changes: 13 additions & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5592,6 +5592,15 @@ dependencies = [
"walkdir",
]

[[package]]
name = "re_capabilities"
version = "0.21.0-alpha.1+dev"
dependencies = [
"document-features",
"egui",
"static_assertions",
]

[[package]]
name = "re_case"
version = "0.21.0-alpha.1+dev"
Expand Down Expand Up @@ -5788,6 +5797,7 @@ dependencies = [
"image",
"itertools 0.13.0",
"nohash-hasher",
"re_capabilities",
"re_chunk_store",
"re_entity_db",
"re_format",
Expand Down Expand Up @@ -6786,6 +6796,7 @@ dependencies = [
"re_blueprint_tree",
"re_build_info",
"re_build_tools",
"re_capabilities",
"re_chunk",
"re_chunk_store",
"re_chunk_store_ui",
Expand Down Expand Up @@ -6867,6 +6878,7 @@ dependencies = [
"nohash-hasher",
"once_cell",
"parking_lot",
"re_capabilities",
"re_chunk",
"re_chunk_store",
"re_data_source",
Expand Down Expand Up @@ -7167,6 +7179,7 @@ dependencies = [
"re_analytics",
"re_build_info",
"re_build_tools",
"re_capabilities",
"re_chunk",
"re_chunk_store",
"re_crash_handler",
Expand Down
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ rerun-cli = { path = "crates/top/rerun-cli", version = "=0.21.0-alpha.1", defaul

# crates/utils:
re_analytics = { path = "crates/utils/re_analytics", version = "=0.21.0-alpha.1", default-features = false }
re_capabilities = { path = "crates/utils/re_capabilities", version = "=0.21.0-alpha.1", default-features = false }
re_case = { path = "crates/utils/re_case", version = "=0.21.0-alpha.1", default-features = false }
re_crash_handler = { path = "crates/utils/re_crash_handler", version = "=0.21.0-alpha.1", default-features = false }
re_error = { path = "crates/utils/re_error", version = "=0.21.0-alpha.1", default-features = false }
Expand Down
8 changes: 7 additions & 1 deletion crates/top/rerun-cli/src/bin/rerun.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,17 @@ fn main() -> std::process::ExitCode {
}

fn main_impl() -> std::process::ExitCode {
let main_thread_token = rerun::MainThreadToken::i_promise_i_am_on_the_main_thread();
re_log::setup_logging();

let build_info = re_build_info::build_info!();

let result = rerun::run(build_info, rerun::CallSource::Cli, std::env::args());
let result = rerun::run(
main_thread_token,
build_info,
rerun::CallSource::Cli,
std::env::args(),
);

match result {
Ok(exit_code) => std::process::ExitCode::from(exit_code),
Expand Down
3 changes: 2 additions & 1 deletion crates/top/rerun/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -124,18 +124,19 @@ web_viewer = ["server", "dep:re_web_viewer_server", "re_sdk?/web_viewer"]

[dependencies]
re_build_info.workspace = true
re_capabilities.workspace = true
re_chunk.workspace = true
re_crash_handler.workspace = true
re_entity_db.workspace = true
re_error.workspace = true
re_format.workspace = true
re_log_encoding.workspace = true
re_log_types.workspace = true
re_video.workspace = true
re_log.workspace = true
re_memory.workspace = true
re_smart_channel.workspace = true
re_tracing.workspace = true
re_video.workspace = true

anyhow.workspace = true
document-features.workspace = true
Expand Down
6 changes: 5 additions & 1 deletion crates/top/rerun/src/commands/entrypoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,7 @@ enum Command {
// It would be nice to use [`std::process::ExitCode`] here but
// then there's no good way to get back at the exit code from python
pub fn run<I, T>(
main_thread_token: crate::MainThreadToken,
build_info: re_build_info::BuildInfo,
call_source: CallSource,
args: I,
Expand Down Expand Up @@ -595,7 +596,7 @@ where
}
}
} else {
run_impl(build_info, call_source, args)
run_impl(main_thread_token, build_info, call_source, args)
};

match res {
Expand All @@ -620,6 +621,7 @@ where
}

fn run_impl(
main_thread_token: crate::MainThreadToken,
_build_info: re_build_info::BuildInfo,
call_source: CallSource,
args: Args,
Expand Down Expand Up @@ -836,8 +838,10 @@ fn run_impl(
} else {
#[cfg(feature = "native_viewer")]
return re_viewer::run_native_app(
main_thread_token,
Box::new(move |cc| {
let mut app = re_viewer::App::new(
main_thread_token,
_build_info,
&call_source.app_env(),
startup_options,
Expand Down
2 changes: 2 additions & 0 deletions crates/top/rerun/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ pub use re_entity_db::external::re_chunk_store::{
};
pub use re_log_types::StoreKind;

pub use re_capabilities::MainThreadToken;

/// To register a new external data loader, simply add an executable in your $PATH whose name
/// starts with this prefix.
// NOTE: this constant is duplicated in `re_data_source` to avoid an extra dependency here.
Expand Down
6 changes: 5 additions & 1 deletion crates/top/rerun/src/native_viewer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ use re_log_types::LogMsg;
///
/// ⚠️ This function must be called from the main thread since some platforms require that
/// their UI runs on the main thread! ⚠️
pub fn show(msgs: Vec<LogMsg>) -> re_viewer::external::eframe::Result {
pub fn show(
main_thread_token: crate::MainThreadToken,
msgs: Vec<LogMsg>,
) -> re_viewer::external::eframe::Result {
if msgs.is_empty() {
re_log::debug!("Empty array of msgs - call to show() ignored");
return Ok(());
Expand All @@ -18,6 +21,7 @@ pub fn show(msgs: Vec<LogMsg>) -> re_viewer::external::eframe::Result {

let startup_options = re_viewer::StartupOptions::default();
re_viewer::run_native_viewer_with_messages(
main_thread_token,
re_build_info::build_info!(),
re_viewer::AppEnvironment::from_store_source(&store_source),
startup_options,
Expand Down
35 changes: 35 additions & 0 deletions crates/utils/re_capabilities/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
[package]
name = "re_capabilities"
authors.workspace = true
description = "Capability tokens for the Rerun code base."
edition.workspace = true
homepage.workspace = true
include.workspace = true
license.workspace = true
publish = true
readme = "README.md"
repository.workspace = true
rust-version.workspace = true
version.workspace = true

[lints]
workspace = true

[package.metadata.docs.rs]
all-features = true


[features]
default = []

## Enable constructing the [`MainThreadToken`] from an [`egui::Ui`].
egui = ["dep:egui"]


[dependencies]
# Internal dependencies:

# External dependencies:
document-features.workspace = true
egui = { workspace = true, default-features = false, optional = true }
static_assertions.workspace = true
11 changes: 11 additions & 0 deletions crates/utils/re_capabilities/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# re_capabilities

Part of the [`rerun`](https://github.com/rerun-io/rerun) family of crates.

[![Latest version](https://img.shields.io/crates/v/re_capabilities.svg)](https://crates.io/crates/re_capabilitiescrates/utils/)
[![Documentation](https://docs.rs/re_capabilities/badge.svg?speculative-link)](https://docs.rs/re_capabilities?speculative-link)
![MIT](https://img.shields.io/badge/license-MIT-blue.svg)
![Apache](https://img.shields.io/badge/license-Apache-blue.svg)

Specifies capability tokens, required by different parts of the code base.
These are tokens passed down the call tree, to explicitly allow different capabilities in different parts of the code base.
19 changes: 19 additions & 0 deletions crates/utils/re_capabilities/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//! Specifies capability tokens, required by different parts of the code base.
//! These are tokens passed down the call tree, to explicitly allow different capabilities in different parts of the code base.
//!
//! For instance, the [`MainThreadToken`] is taken by argument in functions that needs to run on the main thread.
//! By requiring this token, you guarantee at compile-time that the function is only called on the main thread.
//!
//! All capability tokens should be created in the top-level of the call tree,
//! (i.e. in `fn main`) and passed down to all functions that require it.
//! That way you can be certain in what an area of code is allowed to do.
//!
//! See [`cap-std`](https://crates.io/crates/cap-std) for another capability-centric crate.
//!
//! ## Feature flags
#![doc = document_features::document_features!()]
//!
mod main_thread_token;

pub use main_thread_token::MainThreadToken;
60 changes: 60 additions & 0 deletions crates/utils/re_capabilities/src/main_thread_token.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
use static_assertions::assert_not_impl_any;

/// A token that (almost) proves we are on the main thread.
///
/// Certain operations are only allowed on the main thread.
/// These operations should require this token.
/// For instance, any function using file dialogs (e.g. using [`rfd`](https://docs.rs/rfd/latest/rfd/)) should require this token.
///
/// The token should only be constructed in `fn main`, using [`MainThreadToken::i_promise_i_am_on_the_main_thread`],
/// and then be passed down the call tree to where it is needed.
/// [`MainThreadToken`] is neither `Send` nor `Sync`,
/// thus guaranteeing that it cannot be found in other threads.
///
/// Of course, there is nothing stopping you from calling [`MainThreadToken::i_promise_i_am_on_the_main_thread`] from a background thread,
/// but PLEASE DON'T DO THAT.
/// In other words, don't use this as a guarantee for unsafe code.
///
/// There is also [`MainThreadToken::from_egui_ui`] which uses the implicit guarantee of egui
/// (which _usually_ is run on the main thread) to construct a [`MainThreadToken`].
/// Use this only in a code base where you are sure that egui is running only on the main thread.
#[derive(Clone, Copy)]
pub struct MainThreadToken {
/// Prevent from being sent between threads.
///
/// Workaround until `impl !Send for X {}` is stable.
_dont_send_me: std::marker::PhantomData<*const ()>,
}

impl MainThreadToken {
/// Only call this from `fn main`, or you may get weird runtime errors!
pub fn i_promise_i_am_on_the_main_thread() -> Self {
// On web there is no thread name.
// On native the thread-name is always "main" in Rust,
// but there is nothing preventing a user from also naming another thread "main".
// In any case, since `MainThreadToken` is just best-effort, we only check this in debug builds.
#[cfg(not(target_arch = "wasm32"))]
debug_assert_eq!(std::thread::current().name(), Some("main"),
"DEBUG ASSERT: Trying to construct a MainThreadToken on a thread that is not the main thread!"
);

Self {
_dont_send_me: std::marker::PhantomData,
}
}

/// We _should_ only create an [`egui::Ui`] on the main thread,
/// so having it is good enough to "prove" that we are on the main thread.
///
/// Use this only in a code base where you are sure that egui is running only on the main thread.
///
/// In theory there is nothing preventing anyone from creating a [`egui::Ui`] on another thread,
/// but practice that is unlikely (or intentionally malicious).
#[cfg(feature = "egui")]
pub fn from_egui_ui(_ui: &egui::Ui) -> Self {
Self::i_promise_i_am_on_the_main_thread()
}
}

assert_not_impl_any!(MainThreadToken: Send, Sync);
assert_not_impl_any!(&MainThreadToken: Send, Sync);
2 changes: 1 addition & 1 deletion crates/utils/re_crash_handler/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

Part of the [`rerun`](https://github.com/rerun-io/rerun) family of crates.

[![Latest version](https://img.shields.io/crates/v/re_crash_handler.svg)](https://crates.io/crates/utils/re_crash_handler)
[![Latest version](https://img.shields.io/crates/v/re_crash_handler.svg)](https://crates.io/crates/re_crash_handler)
[![Documentation](https://docs.rs/re_crash_handler/badge.svg)](https://docs.rs/re_crash_handler)
![MIT](https://img.shields.io/badge/license-MIT-blue.svg)
![Apache](https://img.shields.io/badge/license-Apache-blue.svg)
Expand Down
1 change: 1 addition & 0 deletions crates/viewer/re_data_ui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ workspace = true
all-features = true

[dependencies]
re_capabilities = { workspace = true, features = ["egui"] }
re_chunk_store.workspace = true
re_entity_db.workspace = true
re_format.workspace = true
Expand Down
1 change: 1 addition & 0 deletions crates/viewer/re_data_ui/src/blob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ pub fn blob_preview_and_save_ui(
}

ctx.command_sender.save_file_dialog(
re_capabilities::MainThreadToken::from_egui_ui(ui),
&file_name,
"Save blob".to_owned(),
blob.to_vec(),
Expand Down
8 changes: 6 additions & 2 deletions crates/viewer/re_data_ui/src/instance_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -389,8 +389,12 @@ fn image_download_button_ui(
.map_or("image", |name| name.unescaped_str())
.to_owned()
);
ctx.command_sender
.save_file_dialog(&file_name, "Save image".to_owned(), png_bytes);
ctx.command_sender.save_file_dialog(
re_capabilities::MainThreadToken::from_egui_ui(ui),
&file_name,
"Save image".to_owned(),
png_bytes,
);
}
Err(err) => {
re_log::error!("{err}");
Expand Down
1 change: 1 addition & 0 deletions crates/viewer/re_viewer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ grpc = ["re_data_source/grpc", "dep:re_grpc_client"]
# Internal:
re_blueprint_tree.workspace = true
re_build_info.workspace = true
re_capabilities.workspace = true
re_chunk.workspace = true
re_chunk_store.workspace = true
re_component_ui.workspace = true
Expand Down
Loading

0 comments on commit db18508

Please sign in to comment.