Skip to content

Conversation

@Sean-Der
Copy link
Contributor

Don't kill the entire connection because TCP has errored

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

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

I think it works but it is not efficient since instead of marking the pair as failed immediately if the TCP connection fails, it waits for timeout anyway.

You should at least fix the number of retransmissions for TCP. currently it seems to me that agent_arm_transmission() sets it to 6 like for UDP, meaning you wait for a long time and call conn_tcp_connect() multiple times uselessly before timing out.

Don't kill the entire connection because TCP has errored
Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

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

I'm merging this but it looks like there are still issues introduced by TCP support, for instance a partial message makes the polling thread basically busy-wait and use 100% CPU, and it crashes when TCP connects if an ICE server is set (fixed by #316).

@paullouisageneau paullouisageneau merged commit 4708e90 into paullouisageneau:master Sep 22, 2025
3 checks passed
@Sean-Der
Copy link
Contributor Author

Thank you @paullouisageneau

Do you have recommendations/ideas on unit tests or anything else I can do to catch more stuff?

@paullouisageneau
Copy link
Owner

@Sean-Der I looked into it and ended up making a bunch of fixes in #317. The most important one is that the ICE-TCP framing protocol must be implemented with buffering, since as soon as the TCP buffer is full send() may result in a short write. I'm not finished, as there are still some issues like retransmissions happening over TCP, I'll be back to it soon.

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.

2 participants