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

handling send to socket error #31

Merged
merged 2 commits into from
Jun 22, 2023

Conversation

biryukovmaxim
Copy link
Contributor

@biryukovmaxim biryukovmaxim commented Jun 21, 2023

fix #27
create SendError enum, BroadcastError
handle these errors

@Totodore I left todos in places where I don't know how to handle error correctly

@biryukovmaxim biryukovmaxim changed the title Draft: handling send to socket error handling send to socket error Jun 21, 2023
@Totodore
Copy link
Owner

I didn't realized that returning the packet to send when there is an error implies to deserialize it again or returning a raw SendPacket. These two solutions are not optimal...

We could tweak the emit fn so it could directly emit SendPackets. Like that the user would be able to try to send the failed packet again

Otherwise, the easiest solution would be to return an error but without the packet...

What do you think @biryukovmaxim ?

@biryukovmaxim biryukovmaxim marked this pull request as draft June 22, 2023 10:38
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 this solution.
I don't see what we could do more to have better error handling with packet returned

@Totodore Totodore marked this pull request as ready for review June 22, 2023 12:06
@Totodore Totodore merged commit a714e4b into Totodore:main Jun 22, 2023
@biryukovmaxim
Copy link
Contributor Author

Noo plz, let me 20 minutes. I will show

@Totodore
Copy link
Owner

Totodore commented Jun 22, 2023

Sorry, I'm reverting the pr 😅

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