fix: implement relay websocket connection state #128
Conversation
geekbrother
force-pushed
the
max/fix/add_connection_state
branch
from
a95fcb1 to
a0bf2f2
Compare
| @@ -2,6 +2,12 @@ | |||
| All notable changes to this project will be documented in this file. See [conventional commits](https://www.conventionalcommits.org/) for commit guidelines. | |||
|
|
|||
| - - - | |||
| ## v0.15.1 - 2023-10-17 | |||
There was a problem hiding this comment.
Once my PR is merged can you reset this CHANGELOG, revert the version in Cargo.toml, and delete any created GitHub releases before merging?
| client, | ||
| client_state, | ||
| app_state, |
There was a problem hiding this comment.
This is a lot to have to pass around. Can we wrap all 3 of these in a struct (maybe called RobustRelayClient) and put publish_message(&self) in its implementation?
| #[derive(Debug, Clone)] | ||
| pub enum WebSocketClientState { | ||
| Connected, | ||
| Disconnected, |
There was a problem hiding this comment.
non-blocking: I'm a little scared of this state being here. It doesn't look like this is ever set other than during startup, but it would be nice if there was an approach that avoided this extra state so it was always either Connected or Connecting.
| } | ||
|
|
||
| if let Err(e) = wsclient | ||
| .publish(topic.clone(), message, tag, ttl, prompt) |
There was a problem hiding this comment.
It's possible the relay gets disconnected after getting current_client_state_clone above and calling .publish() here. I think we should always assume it is connected, call .publish(), and if there is a connection error (not just any error), then start waiting for the reconnect to succeed.
Furthermore by calling respawn_connection() in this function and with the race condition mentioned above, it's possible for multiple calls to also call respawn_connection() for the same disconnect. I'd suggest putting respawn_connection() inside of the handler for wsclient::RelayClientEvent::Disconnected instead so that it happens 1 time and also automatically whenever the relay is disconnected, not just when a publish fails.
I don't think we need a connected/not-connected state variable actually, just a RwLock<Notify>. Whenever a disconnected-related publish() error happens, then it does a read-lock and calls notified(). And whenever the Disconnected and respawn_connection() complete, then it does a write-lock, calls notify_waiters(), replaces the Notify with a fresh one, and unlocks.
The assumption is that a disconnected-related publish() errors are strongly correlated with wsclient::RelayClientEvent::Disconnected events.
| @@ -23,7 +24,11 @@ use { | |||
| }, | |||
| serde::{Deserialize, Serialize}, | |||
| sha2::Sha256, | |||
| std::{sync::Arc, time::Instant}, | |||
| std::{ | |||
| sync::{Arc, Mutex}, | |||
There was a problem hiding this comment.
Shouldn't we always use tokio Mutex not blocking ones?
|
After discussion, we've decided to switch publishing in handlers to the HTTP relay client. |
Description
This PR introduces the following changes:
Connectingstate includes a tokio notifier to notify waiting for the connection/reconnection threads.Resolves #74
How Has This Been Tested?
The concept was tested by the simple isolated code, and implementation was tested using the unit and integration tests.
Due Diligence