Spawn via $PATH 1: Rust implementation#3996
Conversation
$PATH 1: Rust implementation
emilk
left a comment
There was a problem hiding this comment.
Lovely to finally get the ball rolling on this!
a966f54 to
91f6edb
Compare
91f6edb to
72b1eca
Compare
crates/re_sdk/src/spawn.rs
Outdated
| if TcpStream::connect_timeout(&connect_addr, Duration::from_millis(1)).is_ok() { | ||
| re_log::info!( | ||
| addr = %opts.listen_addr(), | ||
| "A process is already listening at this address. Assuming it's a Rerun Viewer." | ||
| ); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
To return Ok when perhaps there is a completely unrelated (non-rerun) process already occupying the port seems like trouble waiting to happen…
To me it makes sense it it is a special error (SpawnError::AddressAlreadyInUse) and have the caller do the assuming instead, but it's not a blocker. At least the logging is loud and clear!
There was a problem hiding this comment.
We've used the same trick in the Python SDK for months without issue.
I'll make an issue to introduce a proper Rerun handshake, but this is out of the scope of this PR.
| /// use std::f32::consts::TAU; | ||
| /// | ||
| /// fn main() -> Result<(), Box<dyn std::error::Error>> { | ||
| /// let (rec, storage) = rerun::RecordingStreamBuilder::new("rerun_example_arrow3d").memory()?; |
There was a problem hiding this comment.
All of our examples now pass None as the flush_timeout. Reading the docs:
flush_timeoutis the minimum time the [TcpSink][crate::log_sink::TcpSink] will
wait during a flush before potentially dropping data. Note: PassingNonehere can cause a
call toflushto block indefinitely if a connection cannot be established.
At the same time we have re_sdk::default_flush_timeout() which return Some(2s)
So we are effectively teaching users (via example) not to use the default. I think this is weird.
At the same time, the examples in the docs for spawn and spawn_opts DOES make use of this, so even our examples are inconsistent.
What do we want our examples to behave? What do we want our users to do?
Without having thought through the actual flushing consequences, I would suggest that spawn() takes no arguments so that we pick the same default for all our examples (and for our users!).
There was a problem hiding this comment.
spawn is effectively just a wrapper around connect so I want them to be consistent, and users definitely need to be able to customize this...
We teach users to use re_sdk::default_flush_timeout() anytime we mention connect(), I'll do the same for spawn() then.
There was a problem hiding this comment.
…or we use the same pattern of connect() and connect_opts(…).
I really wish Rust has default arguments 😭
There was a problem hiding this comment.
…or we use the same pattern of
connect()andconnect_opts(…).
That could be quite good. I'll give it a shot in a follow up.
I really wish Rust has default arguments 😭
:yes:
a6c81dc to
eff9ed5
Compare
…mples (#3997) - Get rid of the old thread-based `spawn` functionality. - Redesign `RerunArgs` to get rid of the awful callback while still dealing with Tokio's TLS shenanigans. - Update all tests and examples. The new `RerunArgs` combined with the new `spawn` from PATH now make for a pretty nice experience: ```rust let args = Args::parse(); let (rec, _serve_guard) = args.rerun.init("my_app")?; // do stuff with rec ``` --- Spawn via `$PATH` series: - #3996 - #3997 - #3998 --- - Fixes #2109
- Introduces standalone `rr_spawn(rr_spawn_options)` C API that allows one to start a Rerun Viewer process ready to listen for TCP connections, as well as the associated `rr_recording_stream` integration. - Introduces standalone `rerun::spawn()` C++ API that allows one to start a Rerun Viewer process ready to listen for TCP connections, as well as the associated `RecordingStream` integration. https://github.com/rerun-io/rerun/assets/2910679/8ae5003a-78b2-4e75-91d3-6dc4b8dd22ac --- Spawn via `$PATH` series: - #3996 - #3997 - #3998 --- - Fixes #3757 - Fixes #3942
Introduces standalone
rerun::spawn(SpawnOptions)API that allows one to start a Rerun Viewer process ready to listen for TCP connections, as well as the associatedRecordingStreamintegration.23-10-25_11.40.45.patched.mp4
Spawn via
$PATHseries:$PATH1: Rust implementation #3996$PATH2: Redesignclapintegration and clean up all examples #3997$PATH3: C and C++ implementations #3998Checklist