Making the chat server an actor. #2793
Conversation
dessalines
commented
Mar 30, 2023
dessalines
commented
- Fixes Memory leak causing server crashes #2778
- Inability to make API routes independent of federation data. #2787
| context: ContextData<LemmyContext>, | ||
| ) -> Result<String, LemmyError> { | ||
| let rate_limiter = context.settings_updated_channel(); |
There was a problem hiding this comment.
The RateLimitCell was already in the context, so I hope its okay to do this.
|
Ugh, I can't figure out why this is failing. It passes fine locally. https://woodpecker.join-lemmy.org/LemmyNet/lemmy/pipeline/89/12 |
|
The same error also happened in the PR to enable Woodpecker. So its somehow related to it. I would try to debug it in another branch by disabling all other CI steps and logging the full HTTP response thats causing the error. |
src/api_routes_websocket.rs
Outdated
| "Websocket join with IP: {} has been rate limited.", | ||
| &client_ip | ||
| ); | ||
| return Ok(HttpResponse::Unauthorized().finish()); |
There was a problem hiding this comment.
Use HTTP 429, otherwise its just confusing.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/429
src/api_routes_websocket.rs
Outdated
| context: web::Data<LemmyContext>, | ||
| rate_limiter: web::Data<RateLimitCell>, | ||
| /// How often heartbeat pings are sent | ||
| const HEARTBEAT_INTERVAL: Duration = Duration::from_secs(5); |
There was a problem hiding this comment.
This seems way too much, especially for mobile. Isnt 30 or 60 seconds enough?
There was a problem hiding this comment.
Sure, I'll increase to 30 and 60s, from here: https://stackoverflow.com/questions/12815231/controlling-the-heartbeat-timeout-from-the-client-in-socket-io
There was a problem hiding this comment.
Not sure where you see 30 and 60, the first answer says heartbeat interval = 25s and heartbeat timeout = 60s.
|
|
||
| impl LemmyContext { | ||
| #[tracing::instrument(skip_all)] | ||
| pub async fn send_post_ws_message( |
There was a problem hiding this comment.
Good idea to move these methods into LemmyContext. But leave the generic parameter like before, its annoying to have to_string() all over the place.
There was a problem hiding this comment.
The generic op was way too complicated, mainly because for some reason the WebSocketOperation is split into 3 different enums. There was some hacks that I was able to remove by doing this.
There was a problem hiding this comment.
Which hacks? I think the generics are way cleaner on the call side, but maybe not in the websocket code itself.
There was a problem hiding this comment.
Another approach you can use is to take a generic param like <OP: Into<String>>. Then you can treat it as string in the websocket code, while call sites dont have to put to_string() everywhere.
There was a problem hiding this comment.
I found a way to do it, I'll have this uploaded soon.
|
Aaaand woodpecker is now passing for no apparent reason lol. I'll take it. |
