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

SocketIO: Lots of panics on unwrap in case of there are lots of clients #27

Closed
biryukovmaxim opened this issue Jun 20, 2023 · 5 comments · Fixed by #31 or #34
Closed

SocketIO: Lots of panics on unwrap in case of there are lots of clients #27

biryukovmaxim opened this issue Jun 20, 2023 · 5 comments · Fixed by #31 or #34
Labels
bug Something isn't working

Comments

@biryukovmaxim
Copy link
Contributor

error:

panicked at '14called `Result::unwrap()` on an `Err` value: PoisonError { .. }
', thread 'socketioxide/src/ns.rstokio-runtime-worker:' panicked at '104called `Result::unwrap()` on an `Err` value: PoisonError { .. }:', 14socketioxide/src/ns.rs
:104:14

How to reproduce:
run socketio server(i used main) cargo run --release --bin socketioxide-e2e
run js clients client.zip

For me it reproduces even with 100 clients.

also I locally tried to replace RWlock to Dashmap and errors dissappear

@Totodore
Copy link
Owner

Iit is weird that there are poisoned locks because locks appear only in the get_socket() fn in EngineIo and when inserting a new socket. I'm curious about the poisoned lock origin

If it works with dashmap without too much overhead we could try to switch

@biryukovmaxim
Copy link
Contributor Author

biryukovmaxim commented Jun 20, 2023

my logs was a consequence, not an error
so the real issie is unwrap here

@Totodore
Copy link
Owner

Ah ! You found the real issue :). We need to fix the TODO by mapping the error to the returned Result rather than unwrapping.
I think this happened if the socket is being closed and that you are sending data in the meantime.

@biryukovmaxim
Copy link
Contributor Author

Ah ! You found the real issue :). We need to fix the TODO by mapping the error to the returned Result rather than unwrapping. I think this happened if the socket is being closed and that you are sending data in the meantime.

Also it's related to my client example, it tries to send 10000 messages asynchronously, but default buffer of packets only 128, when I limited clients to send no more than 120 messages - all was good.

I will return error instead of unwrap, but actually we should think how to handle it correctly:
in case of disconnect we should ignore this error
but for all other situations mb we should just skip message not making failed all upper level function

@Totodore
Copy link
Owner

Yes, the best would be to have an error type with a generic that returns the message sent in case of error. Like in the channel errors.

It would imply to create a new enum SendError for example.

@Totodore Totodore reopened this Jun 22, 2023
@Totodore Totodore added the bug Something isn't working label Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants