lsalzman / enet

ENet reliable UDP networking library
MIT License
2.71k stars 669 forks source link

enet_peer_timeout() appears to have incorrect behavior for timeoutLimit parameter #159

Open cgutman opened 3 years ago

cgutman commented 3 years ago

The documentation isn't crystal clear on this, but it seems like the timeoutLimit parameter for the enet_peer_timeout() function does not work as intended.

The documentation regarding the function says the following:

  1. Timeout values use an exponential backoff mechanism, where if a reliable packet is not acknowledge within some multiple of the average RTT plus a variance tolerance, the timeout will be doubled until it reaches a set limit.
  2. If the timeout is thus at this limit and reliable packets have been sent but not acknowledged within a certain minimum time period, the peer will be disconnected.

I have found ENet to behave correctly in accordance with sentence 2 based on the logic here: https://github.com/lsalzman/enet/blob/cf735e639e5c9e3d2d84d71f1dbf789e8c2f3fd0/protocol.c#L1355-L1363

However, I have not found ENet to behave correctly in accordance with sentence 1. As I understand sentence 1 (and based on reading some of the RTO handling code), timeoutLimit is intended to act as a maximum multiplier for the RTO. Once the RTO multiplier reaches the maximum allowed backoff (timeoutLimit), it should not back off further and continue retransmissions with the maximum RTO until timeoutMinimum expires when the connection would be terminated.

For simplicity, consider the following basic scenario:

In this situation, I would expect the transmissions to occur at:

Instead, I see the following behavior:

While the behavior doesn't differ as much in this basic situation, it can be quite detrimental in environments with high latency where the additional exponential backoff steps can start to reach into multiple seconds between retransmissions if left alone. In this case, I would prefer to limit the backoff at 2x or 4x RTT to avoid excessive RTO latency in the face of packet loss with high RTT variance at the cost of perhaps slightly more retransmissions than necessary. It appears that enet_peer_timeout() was designed to do this, but it doesn't currently work as intended.