From 6ae42c67b85ab09a5c79f4d9173f0c54486b9b32 Mon Sep 17 00:00:00 2001 From: Guangqing Chen Date: Wed, 24 Nov 2021 17:49:06 +0800 Subject: [PATCH] [core] Drop msg by TTL even if hasn't ever been sent (#2068) --- docs/API/API-functions.md | 7 +-- srtcore/buffer.cpp | 107 +++++++++++++++++++++----------------- srtcore/buffer.h | 15 +++--- srtcore/core.cpp | 21 +++++--- 4 files changed, 83 insertions(+), 67 deletions(-) diff --git a/docs/API/API-functions.md b/docs/API/API-functions.md index 58b729fad..e1043b9f4 100644 --- a/docs/API/API-functions.md +++ b/docs/API/API-functions.md @@ -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 diff --git a/srtcore/buffer.cpp b/srtcore/buffer.cpp index 7102d09a8..d07080981 100644 --- a/srtcore/buffer.cpp +++ b/srtcore/buffer.cpp @@ -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; } diff --git a/srtcore/buffer.h b/srtcore/buffer.h index 9b6259226..6478e30b9 100644 --- a/srtcore/buffer.h +++ b/srtcore/buffer.h @@ -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); diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 1833b25d8..b140c6422 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -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) @@ -9327,7 +9325,14 @@ std::pair 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