Send TCP protocol header to ignore non-rerun clients

[gurry]

What

Now we send a special header in the TCP connection before sending the protocol version. It helps the server recognize if the client is a rerun client or something else.

Edit
Have changed the approach to send the header after the protocol version. See the discussion in comments.

• Fixes Rerun TCP protocol should be more structured and only log warnings if the incoming connection is from a Rerun SDK #5883

Checklist

• I have read and agree to Contributor Guide and the Code of Conduct
• I've included a screenshot or gif (if applicable)
• I have tested the web demo (if applicable):
  • Using examples from latest main build: rerun.io/viewer
  • Using full set of examples from nightly build: rerun.io/viewer
• The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
• If applicable, add a new check to the release checklist!

• PR Build Summary
• Recent benchmark results
• Wasm size tracking

To run all checks from main, comment on the PR with @rerun-bot full-check.

[gurry]

Somewhat contrary to the original requirement this fix still emits a warning, although the warning says "Unknown client connected" instead of "The version is wrong". Please let me know if I need to silence the warning.

[gurry]

Also please add labels. I am not sure if I can add them or how to do so.

[gurry]

@rerun-bot full-check

[emilk]

Thanks for the PR! Looks good, but yes, let's also catch ConnectionError::UnknownClient and instead of logging at warn, let's log at debug for that particular case

[gurry]

@rerun-bot full-check

[gurry]

Thanks for the PR! Looks good, but yes, let's also catch ConnectionError::UnknownClient and instead of logging at warn, let's log at debug for that particular case

Moved it to debug

[jleibs]

One thing I need to think through prior to merging is how this impacts the 0.15 -> 0.16 migration.

My read of this code is this will break compatibillity in both directions.

Given the existence of the current PROTOCOL_VERSION processing in legacy clients and servers, I think if it might be worth defining the new protocol order as:

<HEADER_VERSION>rerun<PROTOCOL_VERSION>

Where HEADER_VERSION=0 means no rerun bytes, and HEADER_VERSION=1 means check for rerun bytes.

0.15 client -> 0.16 server could then be detected by seeing a HEADER_VERSION of 0 in the stream, in which case it could skip checking for the rerun bytes. And otherwise assume HEADER_VERSION=PROTOCOL_VERSION=0

0.16 client -> 0.15 server would means the client would transmit:

1rerun0<stream>

which the legacy server would see as being a newer protocol and correctly report the error:
"SDK client is using a newer protocol version (1) than the SDK server (0)".

Optionally we could have 0.16 clients continue to transmit HEADER_VERSION=0 for one release cycle, which would would make them backwards compatible with 0.15 (and correctly work with 0.16 due to the above backwards compatibility), and then prior to 0.17 switch them to sending the new style 1rerun0, which can be properly interpreted by 0.16 servers at that point.

[gurry]

Thanks @jleibs.

I had considered compat, but thought it is not going to be a problem in practice because the server and the client are shipped together so they won't go out of sync. But it is a good idea to implement it to avoid surprises nevertheless.

Your suggested approach will definitely work, but I wonder if we could do it without requiring a new field HEADER_VERSION.

What if we did this in 0.16:

• The client sends <PROTOCOL_VERSION>rerun<stream> (notice that there is no HEADER_VERSION) with PROTOCOL_VERSION set to 1
• The server reads PROTOCOL_VERSION as the first field the same way as it does today. If PROTOCOL_VERSION >= 1 it tries to read rerun and rejects the connection if rerun is missing. If PROTOCOL_VERSION < 1 then it behaves exactly as the 0.15 server and does not attempt to read rerun and processes the message as in 0.15.

The different client/server combinations will behave as follows then:

• 0.15 client and 0.16 server: The server sees PROTOCOL_VERSION 0 and behaves exactly like the 0.15 server thus ensuring backward compat
• 0.16 client and 0.15 server: The old server sees PROTOCOL_VERSION 1 and rejects the connection with the "client is newer" warning thus breaking compat
• 0.16 client and 0.16 server: Everything works normally with all unknown clients being recognized and being logged on debug log level.

Obviously the limitation with this approach is that for the second combination (0.16 client/0.15 server) compat will have to break right now. We won't be able to wait until release 0.17 as you have suggested. But on the flip side it's a simpler scheme and IMHO a more natural evolution of the protocol because PROTOCOL_VERSION still remains the first field in the message.

What do you think?

[jleibs]

@gurry that makes sense to me - sounds like a good compromise.

[gurry]

Thanks.

Will implement it in a couple of days as I'll be away.

[jleibs]

Thanks.

Will implement it in a couple of days as I'll be away.

Sounds good -- however, depending on how things are going, one of us might pick up your branch and add that fix tomorrow since we're coming up against our release deadline. Will link here if so to avoid duplicate work.

[gurry]

Sounds good -- however, depending on how things are going, one of us might pick up your branch and add that fix tomorrow since we're coming up against our release deadline. Will link here if so to avoid duplicate work.

I had some time so just did it.

[gurry]

@rerun-bot full-check

[gurry]

The above check failure is during compilation to wasm and it looks like this:

Compiling Rust to wasm in /home/runner/work/rerun/rerun/target_wasm…
/home/runner/work/rerun/rerun> CARGO_ENCODED_RUSTFLAGS="--cfg=web_sys_unstable_apis" RUSTFLAGS="--cfg=web_sys_unstable_apis" "cargo" "build" "--quiet" "--package" "re_viewer" "--lib" "--target" "wasm32-unknown-unknown" "--target-dir" "/home/runner/work/rerun/rerun/target_wasm" "--no-default-features" "--features=analytics"
error[E0432]: unresolved import `re_sdk_comms::ConnectionError`
  --> crates/re_viewer/src/app.rs:5:5
   |
5  | use re_sdk_comms::ConnectionError;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `ConnectionError` in the root
   |
note: found an item that was configured out
  --> /home/runner/work/rerun/rerun/crates/re_sdk_comms/src/lib.rs:20:25
   |
20 | pub use server::{serve, ConnectionError, ServerError, ServerOptions};
   |                         ^^^^^^^^^^^^^^^
   = note: the item is gated behind the `server` feature

For more information about this error, try `rustc --explain E0432`.
error: could not compile `re_viewer` (lib) due to  1 previous error
Error: Failed to build Wasm

It occurs because server::ConnectionError is gated behind the server feature. In fact the mod server itself is gated behind the same feature.

@jleibs, @emilk please suggest how I should address this. Should I move ConnectionError to some other location which is not feature gated? If so, which one would be best? Any pointers will be appreciated.

[emilk]

Thank you!

I've tested it and it works great.