refactor: restructure websocket implementation #235
Conversation
local-server/src/lib.rs
Outdated
| // TODO: Handle both connection errors here and failed responses. | ||
| let (ws_stream, upstream_response) = tokio_tungstenite::connect_async(request) | ||
| .await | ||
| .expect("WebSocket connection failed"); |
There was a problem hiding this comment.
Match on error:
- If it's Err(HttpResponse) - there is a valid http response even though the websocket didn't connect, return the response to the client (such as 404)
- for other errors - Return a real error / 503 ish
There was a problem hiding this comment.
Changed on 0f0e525. Were you thinking of something like that?
local-server/src/lib.rs
Outdated
| Some(upgrade) => { | ||
| let mut url = target_service.url; |
There was a problem hiding this comment.
Maybe we can fix the naming here a bit to match the upstream/downstream terminology? I had a hard time spotting the ws_stream & upgrade distinction after not looking at the code for a while
There was a problem hiding this comment.
Do you mean the naming of url or the upgrade? 🤔
There was a problem hiding this comment.
Mostly the upgrade which becomes - upgrade.on_upgrade(ws::context_handle_socket(ws_stream)); - is upgrade the upstream/downstream and is ws_stream the upstream/downstream
There was a problem hiding this comment.
Maybe something like this? e4a33aa
There was a problem hiding this comment.
And maybe with this f4c4c03 (now I'm confused as well 😅 )
There was a problem hiding this comment.
I think that's right! "the next hop along" is upstream
Use tungstenite for handling websocket on local-server.
With this implementation we should have a more stable handling of the events and specially of the closing.