Resend uTP ACKs that fail to send due to stalling #7113
Conversation
|
I've added it to my list to add a new uTP simulation where the send socket buffer is filled up. I'm not sure the current libsimulator models this yet. |
src/utp_stream.cpp
Outdated
| // if the socket stalled while sending an ack, then send it | ||
| // now if needed | ||
| if (m_deferred_ack && !m_stalled) | ||
| send_ack(); |
There was a problem hiding this comment.
maybe I'm misunderstanding something here. but if the send_pkt()-loop above ended up sending any packets, we don't need to send a dedicated ACK packet. Since the ACK sequence number is always included in the header.
I would have expected this part of the patch to instead do something like: if m_deferred_ack is set, make the first call to send_pkt() be called with the pkt_ack flag. If it returns true, then move on to run the loop that attempts to drain the send buffer.
looking closer i see that m_deferred_ack is actually cleared by send_pkt() if it ends up sending a packet, so I suppose this also works. However, if send_pkt() doesn't send any packets (and doesn't clear m_deferred_ack), there shouldn't be a way for m_stalled to have been set either.
I'm leaning towards thinking that this code might be slightly simpler if it doesn't rely onm_deferred_ack and m_stalled being updated by the call to send_pkt().
| // if the socket stalled while sending an ack then there will be a | ||
| // pending deferred ack. make sure it gets sent out | ||
| if (!m_deferred_ack || send_pkt(pkt_ack)) | ||
| while(send_pkt()); |
There was a problem hiding this comment.
How about this: If there's no outstanding unsent ack then go straight to the send_pkt() loop like normal, otherwise use the pkt_ack flag for the first call, then proceed into the loop if needed. It's much simpler, and I think will achieve the same thing.
Following #7107, uTP packets with payload that failed to send due to a stalled socket should now be sent once the socket becomes writable again. However, this doesn't apply to
ST_STATEpackets (ie payload-less ACKs), which would simply be lost. If another packet isn't sent out in time (updating ourack_nrat the other end) then this will be seen as loss and can trigger a needless resend by the other-end.The idea behind this change is to note that an ack is still needed if the socket stalls while sending an empty payload ACK-only packet. Once the socket is writable again, the deferred ack is then sent out if its still needed.
I wonder if a simulation with a low enough
send_socket_buffer_sizemight be able to trigger this and to confirm that this fix as well as #7107 work as intended.