Bugfix: properly handle cases of stale loss report #1079
Conversation
ethouris
added
Priority: High
Type: Maintenance
srtcore/core.cpp
Outdated
| #ifndef SRT_TEST_DISABLE_KEY_CONTROL_PACKETS | ||
| sendCtrl(UMSG_DROPREQ, &w_packet.m_iMsgNo, seqpair, sizeof(seqpair)); | ||
| #endif |
There was a problem hiding this comment.
I don't think we should add this to the master. This will be forgotten eventually.
I would suggest to create a document, and maybe a patch, that describe how to perform this test.
Roughlt speaking. disable all sendCtrl(UMSG_DROPREQ...
There was a problem hiding this comment.
This sendCtrl must remain here - that's the intention of the fix. The macrodefinition is here so that tests can be made that simulate dropping this packet.
There was a problem hiding this comment.
I was talking about the ifdef statement. Code pollution.
SRT_TEST_DISABLE_KEY_CONTROL_PACKETS can be easily added inside of the sendCtrl() when needed. But they should not exist in master.
There was a problem hiding this comment.
Ah ok, then you are right. Moreover, with more advanced testing we can disable them using some better methods.
The reason for this change is that the case of the "stale loss report" so far wasn't handled at all.
The UDT handler provided only a case when a loss report covers PARTIALLY the already dismissed range from the sender buffer, but hooks up at least to some packets that are in the sender buffer. This normally should never happen, but the socket group code involving sequence number resynchronization has made this more probable to happen.
This fix only increases the probability to fix things up (as a UDP packet can still be lost), but it's required to not make the receiver hang around uselessly in case when it tried to recover packets that have never be predicted to be sent.
The signal to TSBPD in case when the dropping is received is required because otherwise TSBPD may hangup forever when it happens, and TLPKTDROP doesn't release it.
The
SRT_TEST_DISABLE_KEY_CONTROL_PACKETis for testing purposes only - to test what happens when these drop request command packets happen to not be sent.The quick exit at
processDatais here because an odd moment of closing the socket might cause the packet to be uselessly processed. This might not be the problem for single sockets, but could cause additional confusion if it's a member of the group.