johanhelsing
Warn instead of panic on send failure
Hmm... I thought what we discussed on slack was the internal handshake channel? https://github.com/johanhelsing/matchbox/blob/71745ff73484463ea70ab6eca8bac6cf5bdeb07d/matchbox_socket/src/webrtc_socket/native.rs#L522
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.
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.
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 :)
I for one think that this should return a Result<(), Error> or something like it. IMO, if it smells like it could fail, then it should explicitly say it could fail instead of logging or panicking. The client should be able to choose if they panic or gracefully handle the error.
In a multiplayer game, the game shouldn't crash if a packet fails to send. At the very least, it drops you to the main menu with a "Failed to connect to server" message or something (Looking at you, Call Of Duty)
IMO that is a bit too low level for me as a user of the library. I would rather have this method handle the error internally and then f.ex get NetworkInterrupted from this method.
What this panic means is that the internal message delivery to the message loop failed. It indicates something is seriously wrong within the library.
I think this pr just hides another bug, or user error.
Sorry for the slow response, i wanted to try your repro with ggrs, but i haven't had the time, and won't have for the next few weeks.
If you can figure out why the receiver was dropped, without the socket future completing or being dropped, that would be great
Otherwise, there is always try_send. Perhaps the ggrs implementation should use that instead, as an alternate fix to your issue.
What this panic means is that the internal message delivery to the message loop failed. It indicates something is seriously wrong within the library.
I think this pr just hides another bug, or user error.
Sorry for the slow response, i wanted to try your repro with ggrs, but i haven't had the time, and won't have for the next few weeks.
If you can figure out why the receiver was dropped, without the socket future completing or being dropped, that would be great
Otherwise, there is always try_send. Perhaps the ggrs implementation should use that instead, as an alternate fix to your issue.
Ok! Given this info perhaps I can dig into this if I find some time. Perhaps find a minimal reproduction.