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

Better errors when emitting data #184

Closed
wants to merge 12 commits into from
Closed

Better errors when emitting data #184

wants to merge 12 commits into from

Conversation

Totodore
Copy link
Owner

@Totodore Totodore commented Nov 26, 2023

Fix #172

There are two remaining questions :

  • What should be done with binary packets. Because a message can contain N binary payloads it means that we would need N+1 Permits for each emit call.
  • What should be done when broadcasting. What is the performance cost of asking X Permits from N sockets at the same time ? Because packet is encoded only once and then cloned many times to be sent. Maybe it would be better to just clone the data before sending anything so it is possible to send it back to the user.
    Also broadcasting is done through Operators, and Operators are not always used to broadcast, they can be used to simply add a timeout or a binary payload when emitting. So the emit signature needs to be the same from the Operator fn even if it is not broadcasting.
  • For the first problem, rather than using the Permit API, a Semaphore will be added to the engine Socket. With this semaphore it will possible to theoretically "reserve" n slot without creating n Permits.
  • The second problem still needs to be resolved

@Totodore Totodore self-assigned this Nov 26, 2023
@Totodore Totodore added the enhancement New feature or request label Nov 26, 2023
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

clippy found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@Totodore
Copy link
Owner Author

tokio-rs/tokio#6174

self.internal_tx.capacity()
}
/// Reserve n slots in the internal channel
pub fn reserve<'a>(&'a self, n: u32) -> Result<Permit<'a>, TryAcquireError> {

Check warning

Code scanning / clippy

the following explicit lifetimes could be elided: 'a Warning

the following explicit lifetimes could be elided: 'a
self.internal_tx.capacity()
}
/// Reserve n slots in the internal channel
pub fn reserve<'a>(&'a self, n: u32) -> Result<Permit<'a>, TryAcquireError> {

Check warning

Code scanning / clippy

the following explicit lifetimes could be elided: 'a Warning

the following explicit lifetimes could be elided: 'a
self.internal_tx.capacity()
}
/// Reserve n slots in the internal channel
pub fn reserve<'a>(&'a self, n: u32) -> Result<Permit<'a>, TryAcquireError> {

Check warning

Code scanning / clippy

the following explicit lifetimes could be elided: 'a Warning

the following explicit lifetimes could be elided: 'a
}
/// Reserve n slots in the internal channel
pub fn reserve<'a>(&'a self, n: u32) -> Result<Permit<'a>, TryAcquireError> {
Ok(Permit::new(&self.internal_tx, &self.semaphore, n)?)

Check warning

Code scanning / clippy

question mark operator is useless here Warning

question mark operator is useless here

#[cfg(feature = "v3")]
let Payload { data, has_binary } =
payload::encoder(rx, protocol, socket.supports_binary, max_payload).await?;
payload::encoder(rx, &sem, protocol, socket.supports_binary, max_payload).await?;

Check warning

Code scanning / clippy

this expression creates a reference which is immediately dereferenced by the compiler Warning

this expression creates a reference which is immediately dereferenced by the compiler
@@ -287,16 +287,19 @@
}

/// Send the ack response to the client.
pub fn send(self, data: impl Serialize) -> Result<(), SendError> {
pub fn send<T: Serialize>(self, data: T) -> Result<(), SendError<T>> {

Check failure

Code scanning / clippy

enum takes 0 generic arguments but 1 generic argument was supplied Error

enum takes 0 generic arguments but 1 generic argument was supplied
data: impl serde::Serialize,
) -> Result<(), BroadcastError> {
data: T,
) -> Result<(), BroadcastError<T>> {

Check failure

Code scanning / clippy

enum takes 0 generic arguments but 1 generic argument was supplied Error

enum takes 0 generic arguments but 1 generic argument was supplied
) -> Result<(), SendError> {
event: impl Into<String>,
data: T,
) -> Result<(), SendError<T>> {

Check failure

Code scanning / clippy

enum takes 0 generic arguments but 1 generic argument was supplied Error

enum takes 0 generic arguments but 1 generic argument was supplied
@@ -645,7 +644,14 @@
}
}

/// Gets the request info made by the client to connect
pub(crate) fn reserve(&self, n: u32) -> Result<Permit<'_>, SocketError<()>> {

Check failure

Code scanning / clippy

enum takes 0 generic arguments but 1 generic argument was supplied Error

enum takes 0 generic arguments but 1 generic argument was supplied
data: impl serde::Serialize,
) -> Result<(), BroadcastError> {
data: T,
) -> Result<(), BroadcastError<T>> {

Check failure

Code scanning / clippy

enum takes 0 generic arguments but 1 generic argument was supplied Error

enum takes 0 generic arguments but 1 generic argument was supplied
@Totodore Totodore closed this Feb 6, 2024
@Totodore Totodore deleted the feat-try_emit branch February 6, 2024 17:58
@Totodore
Copy link
Owner Author

Totodore commented Feb 6, 2024

Closed in favor of #262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework of errors when emitting data
1 participant