feat: Initialize server websocket #8
No reviewers
Labels
No labels
Good First Issue
Help wanted
Kind/Breaking
Kind/Bug
Kind/Bug Fix
Kind/CI-CD
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Question
Kind/Security
Kind/Testing
Reviewed
Accepted
Reviewed
Confirmed
Reviewed
Declined
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Approved
Status
Blocked
Status
Inactive
Status
Need More Info
Status
Waiting On Author
Status
Waiting On Review
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: OxideTalis/oxidetalis#8
Loading…
Reference in a new issue
No description provided.
Delete branch "awiteb/init-websocket"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Related-to: #2
@ -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;
not sure if this
>= 5
is intentional, since if someone set the ping time to be very short it will always returnfalse
.Good to be in sync with the code in
websocket/mod.rs
or just remove this check, sincepinged_at > ponged_at
should be enoughAgree, because we already sleep in the ping loop
@ -94,0 +153,4 @@
log::info!("Disconnected from {}, inactive", u.public_key);
u.sender.close_channel()
});
self.write().await.retain(|_, u| !is_inactive(u));
both locks can be replaced by one
@ -0,0 +23,4 @@
/// Client websocket event
#[derive(Deserialize, Clone, Debug)]
pub struct ClientEvent {
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
There is duplicate now in
ping/pong
timestamp
, but it would be better in the long run with more events and data.Bro!!! I didn't know about tag-content, is amazing
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 thedata
from it, what do you think?This will output
{"timestamp":4398}
as bytes, we will make the signature from itI would use this
first, we don't need to serialize the whole thing as it would include the
signature
and then we will discard everything except fordata
, so better to serialize only the smallest needed parts.second, we can directly use
to_string
fromValue
, and take the bytes out of it, I think its a bit better sinceto_vec
would call the serializer forValue
small thing, maybe better to rename
event_type
toevent
? shorter and its not just the type, as it contain the data as wellRight, the function above is for
ClientEventType
notEvent
.I'll force push here today.
Ah yes, didn't notice
@ -0,0 +92,4 @@
depot: &Depot,
) -> Result<(), StatusError> {
let nonce_cache = depot.nonce_cache();
let nonce_limit = *depot.nonce_cache_size();
something I noticed, this
nonce_limit
is just passed everywhere without a need.in
NonceCacheExt::add_nonce
, it takeslimit
, 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@ -0,0 +117,4 @@
let (user_ws_sender, mut user_ws_receiver) = ws.split();
let (sender, receiver) = mpsc::unbounded();
let sender = Arc::new(sender);
sender is already
Clone
and is implemented withArc
@ -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;
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 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.
In general, the ping mechanism is good, but added some notes on small parts of it
@ -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(
Error is silently being ignored here
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.5e2e830979
to2318ef8a2d
2318ef8a2d
to6fcc82e622
Closed by mistake 😬
@ -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"])
this is different from
ServerEventType
, there, its using.to_string().into_bytes()
Forget it 😬
@ -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>,
This is not an issue, but I'm curios:
Mutex<NonceCache>
around, we can useNonceCache
and handle locking inside.async
of course, not sure which is better.Actually a good idea, I'll.
I'll see
8609ff3fb7
tod442e73ed7
Nice, approved
Thank you for your review, it is actually helped
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