-
-
Notifications
You must be signed in to change notification settings - Fork 878
Websocket react on URL change #6150
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
Conversation
Changing from 5G (with public URL that also works when at home) to Wi-Fi (with internal IP) doesn't immediately trigger a disconnect for me, it takes the system a while to actively stop using 5G for network access for active connections and disconnect the websocket. With this change, I suppose that may happen sooner from the app's end now instead of waiting for the system to disconnect + the app to notice? |
...otlin/io/homeassistant/companion/android/common/data/websocket/impl/WebSocketCoreImplTest.kt
Outdated
Show resolved
Hide resolved
| // we abruptly cancel the connection since we might be in an insecure state | ||
| connection?.cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Maybe introduce
WebSocketState.CLOSED_URL_CHANGEfor this? - In case the URL changes, do we want the same delay for reconnecting (10s) or none / a shorter delay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's easy for you to test with 5G lemme know how it behaves on your side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be too difficult, I'll test it tomorrow
Interesting, in that case I'm curious if you could test if you see a difference. I only tested changing Wi-Fi to another one. |
d68d1a4 to
456b172
Compare
31120ab to
7b56408
Compare
7b56408 to
665e9ec
Compare
|
Sorry for the late response: testing when switching between 5G/Wi-Fi now shows immediate disconnects, with the system also immediately switching to the new connection when the websocket is restarted, and also immediate disconnects for trusted Wi-Fi/other Wi-Fi. Testing note: if the websocket is returning a null URL it should probably be handled in the WebSocketManager's job to not indicate a connection in the notification while there is actually none, but that is out of scope of this PR. |
Summary
This is an attempt to make the Websocket impl to react to a URL change.
Checklist
Any other notes
This is an attempt because I'm not sure it is actually doing anything, I don't know if there are a use case where the URL change that is not already disconnecting the web socket. Maybe if the internal is still accessible, but changing wifi SSID does disconnect briefly the WS already.