feat: Initialize server websocket #8

Merged
awiteb merged 4 commits from awiteb/init-websocket into master 2024-07-06 13:35:10 +02:00 AGit
Owner

Related-to: #2

Related-to: https://git.4rs.nl/OxideTalis/oxidetalis/issues/2
awiteb added 2 commits 2024-07-05 01:21:51 +02:00
awiteb requested review from Amjad50 2024-07-05 01:39:44 +02:00
awiteb added the
Kind/Feature
label 2024-07-05 01:40:07 +02:00
awiteb added this to the Server Stable Version project 2024-07-05 01:40:12 +02:00
Amjad50 requested changes 2024-07-05 07:19:51 +02:00
Dismissed
@ -94,0 +144,4 @@
async fn disconnect_inactive_users(&self) {
let now = Utc::now().timestamp();
let is_inactive =
|u: &SocketUserData| u.pinged_at > u.ponged_at && now - u.pinged_at.timestamp() >= 5;
Member

not sure if this >= 5 is intentional, since if someone set the ping time to be very short it will always return false.

Good to be in sync with the code in websocket/mod.rs or just remove this check, since pinged_at > ponged_at should be enough

not sure if this `>= 5` is intentional, since if someone set the ping time to be very short it will always return `false`. Good to be in sync with the code in `websocket/mod.rs` or just remove this check, since `pinged_at > ponged_at` should be enough
Author
Owner

since pinged_at > ponged_at should be enough

Agree, because we already sleep in the ping loop

> since pinged_at > ponged_at should be enough Agree, because we already sleep in the ping loop
awiteb marked this conversation as resolved
@ -94,0 +153,4 @@
log::info!("Disconnected from {}, inactive", u.public_key);
u.sender.close_channel()
});
self.write().await.retain(|_, u| !is_inactive(u));
Member

both locks can be replaced by one

self.write().await.retain(|_, u| {
    if is_inactive(u) {
        u.sender.close_channel();
        false
    } else {
        true
    }
})
both locks can be replaced by one ```rust self.write().await.retain(|_, u| { if is_inactive(u) { u.sender.close_channel(); false } else { true } }) ```
awiteb marked this conversation as resolved
@ -0,0 +23,4 @@
/// Client websocket event
#[derive(Deserialize, Clone, Debug)]
pub struct ClientEvent {
Member

The separation between the type and event data and then checking manually with is_of_type looks wrong.

Why not do it in a single single enum to host all events and data

#[derive(Serialize, Deserialize, Debug)]
#[serde(tag = "event", content = "data")]
pub enum ClientEventType {
    Ping { timestamp: u64 },
    Pong { timestamp: u64 },
}

#[derive(Serialize, Deserialize, Debug)]
pub struct ClientEvent {
    #[serde(flatten)]
    pub event_type: ClientEventType,
    pub signature: String,
}

There is duplicate now in ping/pong timestamp, but it would be better in the long run with more events and data.

The separation between the type and event data and then checking manually with `is_of_type` looks wrong. Why not do it in a single single enum to host all events and data ```rust #[derive(Serialize, Deserialize, Debug)] #[serde(tag = "event", content = "data")] pub enum ClientEventType { Ping { timestamp: u64 }, Pong { timestamp: u64 }, } #[derive(Serialize, Deserialize, Debug)] pub struct ClientEvent { #[serde(flatten)] pub event_type: ClientEventType, pub signature: String, } ``` There is duplicate now in `ping/pong` `timestamp`, but it would be better in the long run with more events and data.
Author
Owner

Bro!!! I didn't know about tag-content, is amazing

Bro!!! I didn't know about [tag-content](https://serde.rs/container-attrs.html#tag--content), is amazing
Author
Owner

Is good, but we have a problem. We need the event data for the signature, but we can't extract it directly in this enum.

I written a function that extract the data by serializing the enum variant to serde_json::Value then extract the data from it, what do you think?

impl ClientEventType {
    /// Returns event data as json bytes
    pub fn data(&self) -> Vec<u8> {
        serde_json::to_vec(&serde_json::to_value(self).expect("can't fail")["data"])
            .expect("can't fail")
    }
}

This will output {"timestamp":4398} as bytes, we will make the signature from it

Is good, but we have a problem. We need the event data for the signature, but we can't extract it directly in this enum. I written a function that extract the data by serializing the enum variant to `serde_json::Value` then extract the `data` from it, what do you think? ```rust impl ClientEventType { /// Returns event data as json bytes pub fn data(&self) -> Vec<u8> { serde_json::to_vec(&serde_json::to_value(self).expect("can't fail")["data"]) .expect("can't fail") } } ``` This will output `{"timestamp":4398}` as bytes, we will make the signature from it
Member

I would use this

serde_json::to_value(&self.event_type).unwrap()["data"].to_string().into_bytes())

first, we don't need to serialize the whole thing as it would include the signature and then we will discard everything except for data, so better to serialize only the smallest needed parts.
second, we can directly use to_string from Value, and take the bytes out of it, I think its a bit better since to_vec would call the serializer for Value

I would use this ```rust serde_json::to_value(&self.event_type).unwrap()["data"].to_string().into_bytes()) ``` first, we don't need to serialize the whole thing as it would include the `signature` and then we will discard everything except for `data`, so better to serialize only the smallest needed parts. second, we can directly use `to_string` from `Value`, and take the bytes out of it, I think its a bit better since `to_vec` would call the serializer for `Value`
Member

small thing, maybe better to rename event_type to event? shorter and its not just the type, as it contain the data as well

small thing, maybe better to rename `event_type` to `event`? shorter and its not just the type, as it contain the data as well
Author
Owner

first, we don't need to serialize the whole thing as it would include the signature and then we will discard everything except for data

Right, the function above is for ClientEventType not Event.

I'll force push here today.

> first, we don't need to serialize the whole thing as it would include the signature and then we will discard everything except for data Right, the function above is for `ClientEventType` not `Event`. I'll force push here today.
Member

Ah yes, didn't notice

Ah yes, didn't notice
awiteb marked this conversation as resolved
@ -0,0 +92,4 @@
depot: &Depot,
) -> Result<(), StatusError> {
let nonce_cache = depot.nonce_cache();
let nonce_limit = *depot.nonce_cache_size();
Member

something I noticed, this nonce_limit is just passed everywhere without a need.

in NonceCacheExt::add_nonce, it takes limit, even though both of these items comes from the depot and just passed around in every function, handle_socket -> handle_ws_msg -> verify_signature -> ...... -> add_nonce

A better approach is to make NonceCache a specific struct (not type) that holds both the hashmap and the limit

something I noticed, this `nonce_limit` is just passed everywhere without a need. in `NonceCacheExt::add_nonce`, it takes `limit`, even though both of these items comes from the depot and just passed around in every function, `handle_socket -> handle_ws_msg -> verify_signature -> ...... -> add_nonce` A better approach is to make `NonceCache` a specific struct (not type) that holds both the hashmap and the limit
awiteb marked this conversation as resolved
@ -0,0 +117,4 @@
let (user_ws_sender, mut user_ws_receiver) = ws.split();
let (sender, receiver) = mpsc::unbounded();
let sender = Arc::new(sender);
Member

sender is already Clone and is implemented with Arc

sender is already `Clone` and is implemented with `Arc`
awiteb marked this conversation as resolved
@ -0,0 +206,4 @@
/// Seconds to wait for pongs, before disconnecting the user
const WAIT_FOR_PONGS_SECS: u32 = 10;
/// Seconds to sleep between pings (10 minutes)
const SLEEP_SECS: u32 = 60 * 10;
Member

I think both of these is better to be config as well, more configuration options for admin, 10 mins is good enough of course and probably won't change by most ppl

I think both of these is better to be config as well, more configuration options for admin, 10 mins is good enough of course and probably won't change by most ppl
Author
Owner

more configuration options for admin

I wanted to implement it today, but I felt that this is not something that should be determined by the server administrator? I've never seen a server provide it.

I will not implement it. I don't think that the server administrator must specify the period during which the server sends pings.

> more configuration options for admin I wanted to implement it today, but I felt that this is not something that should be determined by the server administrator? I've never seen a server provide it. I will not implement it. I don't think that the server administrator must specify the period during which the server sends pings.
awiteb marked this conversation as resolved
Member

In general, the ping mechanism is good, but added some notes on small parts of it

In general, the ping mechanism is good, but added some notes on small parts of it
Amjad50 reviewed 2024-07-05 09:11:02 +02:00
@ -94,0 +129,4 @@
let now = Utc::now();
self.write().await.par_iter_mut().for_each(|(_, u)| {
u.pinged_at = now;
let _ = u.sender.unbounded_send(Ok(Message::from(
Member

Error is silently being ignored here

Error is silently being ignored here
Author
Owner

It is ok, because while the connection is in the online list, that mean the user is there, we will enter the user_disconnected function if there is a connection problem with the user.

It is ok, because while the connection is in the online list, that mean the user is there, we will enter the `user_disconnected` function if there is a connection problem with the user.
awiteb marked this conversation as resolved
awiteb force-pushed awiteb/init-websocket from 5e2e830979 to 2318ef8a2d 2024-07-05 20:57:22 +02:00 Compare
awiteb force-pushed awiteb/init-websocket from 2318ef8a2d to 6fcc82e622 2024-07-05 21:00:06 +02:00 Compare
awiteb closed this pull request 2024-07-06 09:17:57 +02:00
awiteb reopened this pull request 2024-07-06 09:18:04 +02:00
Author
Owner

Closed by mistake 😬

Closed by mistake 😬
awiteb requested review from Amjad50 2024-07-06 09:19:21 +02:00
Amjad50 reviewed 2024-07-06 09:57:15 +02:00
@ -0,0 +43,4 @@
impl ClientEventType {
/// Returns event data as json bytes
pub fn data(&self) -> Vec<u8> {
serde_json::to_vec(&serde_json::to_value(self).expect("can't fail")["data"])
Member

this is different from ServerEventType, there, its using .to_string().into_bytes()

this is different from `ServerEventType`, there, its using `.to_string().into_bytes()`
Author
Owner

Forget it 😬

Forget it 😬
awiteb marked this conversation as resolved
Amjad50 reviewed 2024-07-06 10:03:08 +02:00
@ -0,0 +32,4 @@
/// The cache will remove first 10% nonces if the cache limit is reached.
pub struct NonceCache {
/// The nonce cache hashmap, the key is the nonce and the value is the time
cache: HashMap<[u8; 16], i64>,
Member

This is not an issue, but I'm curios:

  • why not put the mutex here inside, instead of passing Mutex<NonceCache> around, we can use NonceCache and handle locking inside.
  • why not use tokio mutex in this, we would need to change many functions to be async of course, not sure which is better.
This is not an issue, but I'm curios: - why not put the mutex here inside, instead of passing `Mutex<NonceCache>` around, we can use `NonceCache` and handle locking inside. - why not use tokio mutex in this, we would need to change many functions to be `async` of course, not sure which is better.
Author
Owner

why not put the mutex here inside

Actually a good idea, I'll.

not sure which is better.

I'll see

> why not put the mutex here inside Actually a good idea, I'll. > not sure which is better. I'll see
awiteb marked this conversation as resolved
awiteb force-pushed awiteb/init-websocket from 8609ff3fb7 to d442e73ed7 2024-07-06 13:08:20 +02:00 Compare
Amjad50 approved these changes 2024-07-06 13:28:25 +02:00
Member

Nice, approved

Nice, approved
Author
Owner

Nice, approved

Thank you for your review, it is actually helped

> Nice, approved Thank you for your review, it is actually helped
awiteb merged commit d442e73ed7 into master 2024-07-06 13:35:10 +02:00
awiteb added the
Status
Approved
label 2024-07-06 13:41:09 +02:00
awiteb removed this from the Server Stable Version project 2024-07-06 13:45:07 +02:00
Author
Owner

I removed this PR from the project board because it is already there, I don't
know how it is actually there, but I think it is because the reference in the
description

image
I removed this PR from the project board because it is already there, I don't know how it is actually there, but I think it is because the reference in the description <img width="248" alt="image" src="/attachments/d78523ee-fc9b-4b39-9046-6bffcbeeb281">
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: OxideTalis/oxidetalis#8
No description provided.