Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(packets): 🐛 Do not bail on "wired" field not found; fix protocol break #2637

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

zmerp
Copy link
Member

@zmerp zmerp commented Jan 17, 2025

No description provided.

@zmerp zmerp changed the title fix(packets): 🐛 Do not bail on "wired" field not found; fix proto… fix(packets): 🐛 Do not bail on "wired" field not found; fix protocol break Jan 17, 2025
Copy link
Collaborator

@The-personified-devil The-personified-devil left a comment

Choose a reason for hiding this comment

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

Not to break even more things, but that flags seems really useless for what it ends up doing, we could just set the protocol to tcp if we're trying to wired. And I think it means that if we have an older client it breaks the integrated adb forwarding (perhaps even in a really unpredictable way), which would otherwise work with a new streamer.

But I think we'd have to keep supplying the flag server-side because otherwise v20.12.0 clients would still trip up over it not being present, even if we just set the protocol to tcp directly.

@The-personified-devil
Copy link
Collaborator

Of course we could also just merge this and only change that with v21, but if that's indeed the only thing holding back compat of older clients with integrated forwarding, then it might be worth doing.

@zmerp
Copy link
Member Author

zmerp commented Jan 17, 2025

we could just set the protocol to tcp if we're trying to wired

The flag was added exactly to avoid having the user change the settings. And yes, for v21 I plan to remove that field because I plan to have the client try both UDP and TCP at the same time

@The-personified-devil
Copy link
Collaborator

The-personified-devil commented Jan 17, 2025

The flag was added exactly to avoid having the user change the settings.

Ofc, but can't we just detect if we're trying to do a wired mode connection in the server connection flow and set that accordingly?

We're currently doing

let is_wired = client_ip.is_loopback();

streaming_caps.is_wired = is_wired;

try_connect();

// client side
if is_wired (
conn_type = tcp
)

// server side
if is_wire (
conn_type = tcp
)

How would doing this not accomplish the same:

let is_wired = client_ip.is_loopback();

try_connect();

if is_wired (
settings.conn_type = tcp
)

@The-personified-devil
Copy link
Collaborator

The exact same place we're sending over the wired flag we're also sending over the session config. (See here).
There's no reason not to change it in the session config we're sending over instead.

@zmerp
Copy link
Member Author

zmerp commented Jan 17, 2025

Not sure i understood you correctly but anyway yeah, the client could have checked if the session contained a client named "client.wired"

@zmerp
Copy link
Member Author

zmerp commented Jan 17, 2025

But for now this fix should be enough. I will remove the flag in v21

@zmerp
Copy link
Member Author

zmerp commented Jan 17, 2025

There's no reason not to change it in the session config we're sending over instead.

By standard, I don't want us to change the session copy we send to the client. Each setting that is inferred at runtime should be negotiated using the negotiation structs

Copy link
Collaborator

@The-personified-devil The-personified-devil left a comment

Choose a reason for hiding this comment

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

Not that I agree, but should be good enough.

@zmerp zmerp merged commit 7304c15 into master Jan 17, 2025
8 checks passed
@zmerp zmerp deleted the fix-protocol-break branch January 17, 2025 18:57
@zmerp zmerp mentioned this pull request Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants