[core] drop packets in the new recv buffer by group recv base

[gou4shi1]

Fix #1964 (comment)

• CUDT::dropToGroupRecvBase() is not placed in CUDT::tsbpd() (as what I have done for the old recv buffer) since this behavior is not related to tsbpd and should also work for message mode.
• CUDT::dropToGroupRecvBase() is placed in sendCtrlAck() since I can't find another better place.
• Some asserts are erased from CRcvBufferNew::dropUpTo() since we need to drop non-empty packets now.
• Update m_numOutOfOrderPackets and m_iFirstReadableOutOfOrder in CRcvBufferNew::dropUpTo() since we need to drop non-empty packets now.

[gou4shi1]

any comment？

[maxsharabayko]

Could you remind me why do we need to drop packets somewhere else apart from when we are reading from the group?
I am not quite sure about placing it in the sendCtrlAck().

[gou4shi1]

Could you remind me why do we need to drop packets somewhere else apart from when we are reading from the group?

sure, you can ref to this comment #1805 (comment)

[maxsharabayko]

For example, my group had two members: A and B.
In one recv() call, A was horse, B was elephant, m_RcvBaseSeqNo was 100, but B only had packets up to 95.
After a while, B received packets from 96-110, but A didn't receive any packet ready to play.
As a result, the tsbpd() of B still tried to PLAYING 96, and the group->updateReadState() failed.

I see. The new receiver buffer does not wait for a packet to be acknowledged to deliver it. Thus I am not sure about placing the "drop to group state" logic in sendCtrlAck(). But I get your point why you don't want it to be in the TSBPD thread neither: this thread does not exist if TSBPD is disabled, meaning the plain message mode will still have the issue.

[gou4shi1]

The new receiver buffer does not wait for a packet to be acknowledged to deliver it. Thus I am not sure about placing the "drop to group state" logic in sendCtrlAck()

Do you mean that you want to decouple ack logic and recv_buffer?

[maxsharabayko]

Do you mean that you want to decouple ack logic and recv_buffer?

It has been already decoupled with the new receiver buffer.

[gou4shi1]

Hi, @maxsharabayko, I have made some changes:

• rebase on [core] Fixed RCV TL drop of packets dropped by SND #2214, so that dropToGroupRecvBase() can just call dropTooLateUpTo()
• also update m_iRcvCurrSeqNo in dropToGroupRecvBase() to ack all not-received packets.

[gou4shi1]

I found that CRcvBufferNew::dropMessage() doesn't update m_numOutOfOrderPackets and m_iFirstNonreadPos, is this intentional?

[gou4shi1]

I found out another solution: call dropTooLateUpTo() in Group::recv().
dropTooLateUpTo() need to drop non-empty units and ack packets over m_iRcvCurrSeqNo.
Then I don't need to change sendCtrlAck() at all.

[gou4shi1]

I found out another solution: call dropTooLateUpTo() in Group::recv().

It's done in #2218.

I found that CRcvBufferNew::dropMessage() doesn't update m_numOutOfOrderPackets and m_iFirstNonreadPos,

It's done in #2222.

[gou4shi1]

I found that ack packets over m_iRcvCurrSeqNo may break senders, e.g.

    if (CSeqNo::seqcmp(ackdata_seqno, CSeqNo::incseq(m_iSndCurrSeqNo)) > 0)
    {
        leaveCS(m_RecvAckLock);
        // this should not happen: attack or bug
        LOGC(gglog.Error,
                log << CONID() << "ATTACK/IPE: incoming ack seq " << ackdata_seqno << " exceeds current "
                    << m_iSndCurrSeqNo << " by " << (CSeqNo::seqoff(m_iSndCurrSeqNo, ackdata_seqno) - 1) << "!");
        m_bBroken        = true;
        m_iBrokenCounter = 0;
        return;
    }

Thus #2218 still need this PR to call dropToGroupRecvBase() in a timer, e.g. sendCtrlAck().

[gou4shi1]

Thus I am not sure about placing the "drop to group state" logic in sendCtrlAck().

I have extracted the change to dropUpTo() into #2221, if this part is acceptable, you can review #2221 firstly :)

I found that ack packets over m_iRcvCurrSeqNo may break senders,

fixed

[gou4shi1]

Thanks for merging #2222, but I'm curious about why #2222 closed this PR?
Maybe my messy chat history is a little bit confused.
To summarize, this PR is to solve the issue mentioned in #1805 (comment)
And I found that receivers can not drop packets over m_iRcvCurrSeqNo, so only dropping in Group::recv() is not enough.
#2222 only to fix the issue I mentioned in this comment #2207 (comment) but not this PR.

[maxsharabayko]

Thanks for merging #2222, but I'm curious about why #2222 closed this PR?

The keyword "fix" followed by issue or RP number in the description of the issue/PR is recognized by GitHub, and automatically closes the relevant PR/issue.
PR #2222 has the following in its description: "Fix #2207".