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

Warn instead of panic on send failure #477

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tedsteen
Copy link
Contributor

@tedsteen tedsteen commented Jan 6, 2025

This closes #476

@johanhelsing
Copy link
Owner

Hmm... I thought what we discussed on slack was the internal handshake channel?

channel_ready.try_send(()).unwrap();

Previously, we agreed that using the socket after dropping the future before was a user error. So if we change the behavior, we need to update the documentation. If we do that, it should probably also be an error, not a warning.

@tedsteen
Copy link
Contributor Author

I noticed after more testing that it happens regardless if I drop a socket or not. F.ex it happens if there is a network disruption during normal operation (see this comment https://discord.com/channels/844211600009199626/1045611882691698688/1323422874182356992). And 99% of the time it is this expect(...) (that I changed in this PR) that was the culprit.

@tedsteen
Copy link
Contributor Author

Perhaps it should be an error, I don't know.. also I don't know if it has other side effects that I haven't considered.. but for my particular case it fixes the problem. I mainly wanted to get the ball rolling and if there is more work needed I'm all ears! Ready to push more commits :)

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.

Panic in WebRtcChannel ::send when there is a network disruption
2 participants