Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement shared custom data #428

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
8 changes: 8 additions & 0 deletions socketio/src/asynchronous/client/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use rust_engineio::{
header::{HeaderMap, HeaderValue},
};
use std::collections::HashMap;
use std::sync::Arc;
use url::Url;

use crate::{error::Result, Event, Payload, TransportType};
Expand Down Expand Up @@ -38,6 +39,7 @@ pub struct ClientBuilder {
pub(crate) max_reconnect_attempts: Option<u8>,
pub(crate) reconnect_delay_min: u64,
pub(crate) reconnect_delay_max: u64,
pub(crate) data: Option<Arc<dyn std::any::Any + Send + Sync>>,
}

impl ClientBuilder {
Expand Down Expand Up @@ -97,9 +99,15 @@ impl ClientBuilder {
max_reconnect_attempts: None,
reconnect_delay_min: 1000,
reconnect_delay_max: 5000,
data: None,
}
}

pub fn data<D: std::any::Any + Send + Sync>(mut self, data: Arc<D>) -> Self {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe name set_data? Didn't find that this is forbidden according to the naming guidelines this crate uses: https://rust-lang.github.io/api-guidelines/naming.html. Only getters omit the prefix.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_data sounds good to me.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why already take the Arc? Can't we get a fat pointer? Or does that play out really badly with lifetimes?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add documentation for the method. IMO this is the perfect place for explaining what data actually is.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! Sure, I mainly forgot to add documentation here honestly.

self.data = Some(data);
self
}

/// Sets the target namespace of the client. The namespace should start
/// with a leading `/`. Valid examples are e.g. `/admin`, `/foo`.
/// If the String provided doesn't start with a leading `/`, it is
Expand Down
15 changes: 15 additions & 0 deletions socketio/src/asynchronous/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ pub struct Client {
auth: Option<serde_json::Value>,
builder: Arc<RwLock<ClientBuilder>>,
disconnect_reason: Arc<RwLock<DisconnectReason>>,
data: Arc<dyn std::any::Any + Send + Sync>,
}

impl Client {
Expand All @@ -87,11 +88,25 @@ impl Client {
nsp: builder.namespace.to_owned(),
outstanding_acks: Arc::new(RwLock::new(Vec::new())),
auth: builder.auth.clone(),
data: builder.data.clone().unwrap_or(Arc::new(())),
builder: Arc::new(RwLock::new(builder)),
disconnect_reason: Arc::new(RwLock::new(DisconnectReason::default())),
})
}

/// Fetches data given by [`ClientBuilder::data`]
pub fn data<D: Send + Sync + 'static>(&self) -> Arc<D> {
self.try_data()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panicing in library code is always a risky thing. We rather want controllable errors (Results).

.expect("Client::data does not match ClientBuilder::data")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhmmm... I don't really like that we need a runtime check here... Wdyt, would it be possible to make this a generic parameter of both the Client and the builder? If this turns out messy, feel free to omit.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic parameter was my main concern, sounds like it'd get messy and fast.

}

/// Attempts to fetch data given by [`ClientBuilder::data`]
///
/// None is returned if data was not given or data does not match [`ClientBuilder::data`]
pub fn try_data<D: Send + Sync + 'static>(&self) -> Option<Arc<D>> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhm, I don't really like the naming here. Usually a try_ prefix implies a Result, which we don't have here. How about we name this method data (returning an option) and the other one data_unchecked which panics? Or we actually don't provide a panic method (because that's usually a bad idea anyways, no library should panic), and just make the upper one return a Result?

Copy link
Author

@kitgxrl kitgxrl May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data_unchecked doesn't really sit right with me. Maybe just try_data returning the option is better suited.

Copy link
Author

@kitgxrl kitgxrl May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or rather just having a custom_data function that returns the option might be better, not try_data

Arc::clone(&self.data).downcast().ok()
}

/// Connects the client to a server. Afterwards the `emit_*` methods can be
/// called to interact with the server.
pub(crate) async fn connect(&self) -> Result<()> {
Expand Down
8 changes: 8 additions & 0 deletions socketio/src/client/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ pub struct ClientBuilder {
opening_headers: Option<HeaderMap>,
transport_type: TransportType,
auth: Option<serde_json::Value>,
data: Option<Arc<dyn std::any::Any + Send + Sync>>,
pub(crate) reconnect: bool,
pub(crate) reconnect_on_disconnect: bool,
// None reconnect attempts represent infinity.
Expand Down Expand Up @@ -98,9 +99,15 @@ impl ClientBuilder {
max_reconnect_attempts: None,
reconnect_delay_min: 1000,
reconnect_delay_max: 5000,
data: None,
}
}

pub fn data<D: std::any::Any + Send + Sync>(mut self, data: Arc<D>) -> Self {
self.data = Some(data);
self
}

/// Sets the target namespace of the client. The namespace should start
/// with a leading `/`. Valid examples are e.g. `/admin`, `/foo`.
pub fn namespace<T: Into<String>>(mut self, namespace: T) -> Self {
Expand Down Expand Up @@ -365,6 +372,7 @@ impl ClientBuilder {
self.on,
self.on_any,
self.auth,
self.data.unwrap_or(Arc::new(())),
)?;
socket.connect()?;

Expand Down
17 changes: 17 additions & 0 deletions socketio/src/client/raw_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use std::time::Duration;
use std::time::Instant;

use crate::socket::Socket as InnerSocket;
use crate::asynchronous::ClientBuilder;

/// Represents an `Ack` as given back to the caller. Holds the internal `id` as
/// well as the current ack'ed state. Holds data which will be accessible as
Expand Down Expand Up @@ -41,6 +42,7 @@ pub struct RawClient {
nsp: String,
// Data send in the opening packet (commonly used as for auth)
auth: Option<Value>,
data: Arc<dyn std::any::Any + Send + Sync>,
}

impl RawClient {
Expand All @@ -54,6 +56,7 @@ impl RawClient {
on: Arc<Mutex<HashMap<Event, Callback<SocketCallback>>>>,
on_any: Arc<Mutex<Option<Callback<SocketAnyCallback>>>>,
auth: Option<Value>,
data: Arc<dyn std::any::Any + Send + Sync>,
) -> Result<Self> {
Ok(RawClient {
socket,
Expand All @@ -62,9 +65,23 @@ impl RawClient {
on_any,
outstanding_acks: Arc::new(Mutex::new(Vec::new())),
auth,
data,
})
}

/// Fetches data given by [`ClientBuilder::data`]
pub fn data<D: Send + Sync + 'static>(&self) -> Arc<D> {
self.try_data()
.expect("RawClient::data does not match ClientBuilder::data")
}

/// Attempts to fetch data given by [`ClientBuilder::data`]
///
/// None is returned if data was not given or data does not match [`ClientBuilder::data`]
pub fn try_data<D: Send + Sync + 'static>(&self) -> Option<Arc<D>> {
Arc::clone(&self.data).downcast().ok()
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as above!

/// Connects the client to a server. Afterwards the `emit_*` methods can be
/// called to interact with the server. Attention: it's not allowed to add a
/// callback after a call to this method.
Expand Down
Loading