Skip to content

Commit

Permalink
[core] Drop msg by TTL even if hasn't ever been sent (#2068)
Browse files Browse the repository at this point in the history
  • Loading branch information
@gou4shi1
gou4shi1 committed Nov 24, 2021
1 parent b99e41c commit 6ae42c6
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 67 deletions.
7 changes: 2 additions & 5 deletions docs/API/API-functions.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1703,11 +1703,8 @@ called function should work.
- `msgttl`: [IN]. In **message** and **live mode** only, specifies the TTL for
sending messages (in `[ms]`). Not used for receiving messages. If this value
is not negative, it defines the maximum time up to which this message should
stay scheduled for sending for the sake of later retransmission. A message
is always sent for the first time, but the UDP packet carrying it may be
(also partially) lost, and if so, lacking packets will be retransmitted. If
the message is not successfully resent before TTL expires, further retransmission
is given up and the message is discarded.
stay scheduled for sending. If TTL has expired, the message sending and further retransmissions are discarded, even
if it has never been sent so far.

- `inorder`: [IN]. In **message mode** only, specifies that sent messages should
be extracted by the receiver in the order of sending. This can be meaningful if
Expand Down
107 changes: 60 additions & 47 deletions srtcore/buffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,57 +397,70 @@ int CSndBuffer::addBufferFromFile(fstream& ifs, int len)
return total;
}

int CSndBuffer::readData(CPacket& w_packet, steady_clock::time_point& w_srctime, int kflgs)
int CSndBuffer::readData(CPacket& w_packet, steady_clock::time_point& w_srctime, int kflgs, int& w_seqnoinc)
{
// No data to read
if (m_pCurrBlock == m_pLastBlock)
return 0;
int readlen = 0;
w_seqnoinc = 0;

while (m_pCurrBlock != m_pLastBlock)
{
// Make the packet REFLECT the data stored in the buffer.
w_packet.m_pcData = m_pCurrBlock->m_pcData;
readlen = m_pCurrBlock->m_iLength;
w_packet.setLength(readlen);
w_packet.m_iSeqNo = m_pCurrBlock->m_iSeqNo;

// XXX This is probably done because the encryption should happen
// just once, and so this sets the encryption flags to both msgno bitset
// IN THE PACKET and IN THE BLOCK. This is probably to make the encryption
// happen at the time when scheduling a new packet to send, but the packet
// must remain in the send buffer until it's ACKed. For the case of rexmit
// the packet will be taken "as is" (that is, already encrypted).
//
// The problem is in the order of things:
// 0. When the application stores the data, some of the flags for PH_MSGNO are set.
// 1. The readData() is called to get the original data sent by the application.
// 2. The data are original and must be encrypted. They WILL BE encrypted, later.
// 3. So far we are in readData() so the encryption flags must be updated NOW because
// later we won't have access to the block's data.
// 4. After exiting from readData(), the packet is being encrypted. It's immediately
// sent, however the data must remain in the sending buffer until they are ACKed.
// 5. In case when rexmission is needed, the second overloaded version of readData
// is being called, and the buffer + PH_MSGNO value is extracted. All interesting
// flags must be present and correct at that time.
//
// The only sensible way to fix this problem is to encrypt the packet not after
// extracting from here, but when the packet is stored into CSndBuffer. The appropriate
// flags for PH_MSGNO will be applied directly there. Then here the value for setting
// PH_MSGNO will be set as is.

if (kflgs == -1)
{
HLOGC(bslog.Debug, log << CONID() << " CSndBuffer: ERROR: encryption required and not possible. NOT SENDING.");
readlen = 0;
}
else
{
m_pCurrBlock->m_iMsgNoBitset |= MSGNO_ENCKEYSPEC::wrap(kflgs);
}

// Make the packet REFLECT the data stored in the buffer.
w_packet.m_pcData = m_pCurrBlock->m_pcData;
int readlen = m_pCurrBlock->m_iLength;
w_packet.setLength(readlen);
w_packet.m_iSeqNo = m_pCurrBlock->m_iSeqNo;

// XXX This is probably done because the encryption should happen
// just once, and so this sets the encryption flags to both msgno bitset
// IN THE PACKET and IN THE BLOCK. This is probably to make the encryption
// happen at the time when scheduling a new packet to send, but the packet
// must remain in the send buffer until it's ACKed. For the case of rexmit
// the packet will be taken "as is" (that is, already encrypted).
//
// The problem is in the order of things:
// 0. When the application stores the data, some of the flags for PH_MSGNO are set.
// 1. The readData() is called to get the original data sent by the application.
// 2. The data are original and must be encrypted. They WILL BE encrypted, later.
// 3. So far we are in readData() so the encryption flags must be updated NOW because
// later we won't have access to the block's data.
// 4. After exiting from readData(), the packet is being encrypted. It's immediately
// sent, however the data must remain in the sending buffer until they are ACKed.
// 5. In case when rexmission is needed, the second overloaded version of readData
// is being called, and the buffer + PH_MSGNO value is extracted. All interesting
// flags must be present and correct at that time.
//
// The only sensible way to fix this problem is to encrypt the packet not after
// extracting from here, but when the packet is stored into CSndBuffer. The appropriate
// flags for PH_MSGNO will be applied directly there. Then here the value for setting
// PH_MSGNO will be set as is.
Block* p = m_pCurrBlock;
w_packet.m_iMsgNo = m_pCurrBlock->m_iMsgNoBitset;
w_srctime = m_pCurrBlock->m_tsOriginTime;
m_pCurrBlock = m_pCurrBlock->m_pNext;

if (kflgs == -1)
{
HLOGC(bslog.Debug, log << CONID() << " CSndBuffer: ERROR: encryption required and not possible. NOT SENDING.");
readlen = 0;
}
else
{
m_pCurrBlock->m_iMsgNoBitset |= MSGNO_ENCKEYSPEC::wrap(kflgs);
}

w_packet.m_iMsgNo = m_pCurrBlock->m_iMsgNoBitset;
w_srctime = m_pCurrBlock->m_tsOriginTime;
m_pCurrBlock = m_pCurrBlock->m_pNext;
if ((p->m_iTTL >= 0) && (count_milliseconds(steady_clock::now() - w_srctime) > p->m_iTTL))
{
LOGC(bslog.Warn, log << CONID() << "CSndBuffer: skipping packet %" << p->m_iSeqNo << " #" << p->getMsgSeq() << " with TTL=" << p->m_iTTL);
// Skip this packet due to TTL expiry.
readlen = 0;
++w_seqnoinc;
continue;
}

HLOGC(bslog.Debug, log << CONID() << "CSndBuffer: extracting packet size=" << readlen << " to send");
HLOGC(bslog.Debug, log << CONID() << "CSndBuffer: extracting packet size=" << readlen << " to send");
break;
}

return readlen;
}
Expand Down
15 changes: 8 additions & 7 deletions srtcore/buffer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,17 +142,18 @@ class CSndBuffer
int addBufferFromFile(std::fstream& ifs, int len);

/// Find data position to pack a DATA packet from the furthest reading point.
/// @param [out] w_packet data packet buffer to fill.
/// @param [out] w_origintime origin time stamp of the message.
/// @param [in] kflags Odd|Even crypto key flag.
/// @param [out] packet the packet to read.
/// @param [out] origintime origin time stamp of the message
/// @param [in] kflags Odd|Even crypto key flag
/// @param [out] seqnoinc the number of packets skipped due to TTL, so that seqno should be incremented.
/// @return Actual length of data read.
int readData(CPacket& w_packet, time_point& w_origintime, int kflgs);
int readData(CPacket& w_packet, time_point& w_origintime, int kflgs, int& w_seqnoinc);

/// Find data position to pack a DATA packet for a retransmission.
/// @param [in] offset offset from the last ACK point (backward sequence number difference)
/// @param [out] w_packet data packet buffer to fill.
/// @param [out] w_origintime origin time stamp of the message
/// @param [out] w_msglen length of the message
/// @param [out] packet the packet to read.
/// @param [out] origintime origin time stamp of the message
/// @param [out] msglen length of the message
/// @return Actual length of data read (return 0 if offset too large, -1 if TTL exceeded).
int readData(const int offset, CPacket& w_packet, time_point& w_origintime, int& w_msglen);

Expand Down
21 changes: 13 additions & 8 deletions srtcore/core.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9219,17 +9219,15 @@ int srt::CUDT::packLostData(CPacket& w_packet, steady_clock::time_point& w_origi
SRT_ASSERT(msglen >= 1);
seqpair[1] = CSeqNo::incseq(seqpair[0], msglen - 1);

HLOGC(qrlog.Debug, log << "IPE: loss-reported packets not found in SndBuf - requesting DROP: "
<< "msg=" << MSGNO_SEQ::unwrap(w_packet.m_iMsgNo) << " msglen=" << msglen << " SEQ:"
<< seqpair[0] << " - " << seqpair[1] << "(" << (-offset) << " packets)");
HLOGC(qrlog.Debug,
log << "loss-reported packets expired in SndBuf - requesting DROP: "
<< "msgno=" << MSGNO_SEQ::unwrap(w_packet.m_iMsgNo) << " msglen=" << msglen
<< " SEQ:" << seqpair[0] << " - " << seqpair[1]);
sendCtrl(UMSG_DROPREQ, &w_packet.m_iMsgNo, seqpair, sizeof(seqpair));

// only one msg drop request is necessary
m_pSndLossList->removeUpTo(seqpair[1]);

// skip all dropped packets
m_pSndLossList->removeUpTo(seqpair[1]);
m_iSndCurrSeqNo = CSeqNo::maxseq(m_iSndCurrSeqNo, seqpair[1]);

continue;
}
else if (payload == 0)
Expand Down Expand Up @@ -9327,7 +9325,14 @@ std::pair<int, steady_clock::time_point> srt::CUDT::packData(CPacket& w_packet)
// It would be nice to research as to whether CSndBuffer::Block::m_iMsgNoBitset field
// isn't a useless redundant state copy. If it is, then taking the flags here can be removed.
kflg = m_pCryptoControl->getSndCryptoFlags();
payload = m_pSndBuffer->readData((w_packet), (origintime), kflg);
int pktskipseqno = 0;
payload = m_pSndBuffer->readData((w_packet), (origintime), kflg, (pktskipseqno));
if (pktskipseqno)
{
// Some packets were skipped due to TTL expiry.
m_iSndCurrSeqNo = CSeqNo::incseq(m_iSndCurrSeqNo, pktskipseqno);
}

if (payload)
{
// A CHANGE. The sequence number is currently added to the packet
Expand Down

0 comments on commit 6ae42c6

Please sign in to comment.