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

Handle send error #34

Merged
merged 16 commits into from
Jun 23, 2023
Merged

Handle send error #34

merged 16 commits into from
Jun 23, 2023

Conversation

biryukovmaxim
Copy link
Contributor

@biryukovmaxim biryukovmaxim commented Jun 22, 2023

use closure to handle fullchannel error, test checking it
resolves #27

socketioxide/src/socket.rs Fixed Show fixed Hide fixed
socketioxide/src/socket.rs Fixed Show fixed Hide fixed
socketioxide/src/socket.rs Fixed Show fixed Hide fixed
socketioxide/src/socket.rs Fixed Show fixed Hide fixed
# Conflicts:
#	socketioxide/src/errors.rs
#	socketioxide/src/socket.rs
@biryukovmaxim
Copy link
Contributor Author

@Totodore that's a solution, but I put one todo, in case of error appears in binary payload send, will fix it when have time

@biryukovmaxim
Copy link
Contributor Author

@Totodore Done. should we mention how many packets remaining inside of the closure?
I think it's mandatory to mention in documentation, that event is successfully delivered if there is no errors, and that resend closure can send some parts

@biryukovmaxim
Copy link
Contributor Author

Also I added dependency to easily implement Debug for Errors including closure
Maybe It's better to implement it manually without unnecessary dependencies, isn't it?

Copy link
Owner

@Totodore Totodore left a comment

Choose a reason for hiding this comment

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

I really like the idea of the closure, I didn't considered this solution before you proposed it but it is very elegant.

socketioxide/src/errors.rs Outdated Show resolved Hide resolved
socketioxide/src/socket.rs Outdated Show resolved Hide resolved
socketioxide/src/socket.rs Outdated Show resolved Hide resolved
socketioxide/src/socket.rs Outdated Show resolved Hide resolved
socketioxide/src/operators.rs Outdated Show resolved Hide resolved
socketioxide/src/handler.rs Outdated Show resolved Hide resolved
socketioxide/src/errors.rs Outdated Show resolved Hide resolved
socketioxide/src/operators.rs Outdated Show resolved Hide resolved
@biryukovmaxim
Copy link
Contributor Author

@Totodore in the last commit I replaced closure with Retryer struct. What do you think about that?

Copy link
Owner

@Totodore Totodore left a comment

Choose a reason for hiding this comment

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

The retryer way is better, I like it

socketioxide/src/socket.rs Outdated Show resolved Hide resolved
socketioxide/src/retryer.rs Outdated Show resolved Hide resolved
socketioxide/src/errors.rs Outdated Show resolved Hide resolved
Copy link
Owner

@Totodore Totodore left a comment

Choose a reason for hiding this comment

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

Merging

@Totodore Totodore merged commit 6a1a642 into Totodore:main Jun 23, 2023
@Totodore
Copy link
Owner

One thing that it is important to note is that due to the possibility to retry to send binary packet it could lead to binary desynchronization. EngineIo binary packets have to be sent next to the event packet, so if there is a retry for a binary packet error and that between the original attempt and the retry there is another event packet sent it will break the mecanism.

@biryukovmaxim
Copy link
Contributor Author

biryukovmaxim commented Jun 23, 2023

So I will think how to handle it. Looks like Socket should own buffer of failed messages to send them firstly or another Arc<Mutex> clone of retryer

@biryukovmaxim biryukovmaxim deleted the handle_send_error branch June 26, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SocketIO: Lots of panics on unwrap in case of there are lots of clients
2 participants