Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RCV Loss List and concurrency in groups #2881

Open
maxsharabayko opened this issue Feb 20, 2024 · 6 comments
Open

RCV Loss List and concurrency in groups #2881

maxsharabayko opened this issue Feb 20, 2024 · 6 comments
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@maxsharabayko
Copy link
Collaborator

This issue is intended to consolidate and track the situation around the RCV loss list and concurrency.

The following issue was submitted for SRT v1.4.4 (#2192 (comment)):

When processData() received 899264404, it was added into the recv buffer, after recvbuf_acklock was unlocked, the tsbpd thread played 899264404 and dropped 899264403, then processData() immediately insert 899264403 back into loss list, after this, getFirstLostSeq() keep return 899264403
To fix the followin issue

PR #2195 introduced the m_iLargestSeq to resolve the bug.

It was later reported in #2873, that if a consecutive loss occurs after more than m_iSeqNoTH to the previous loss record (already removed from the loss list), the loss record is not recorded in the loss list. An ACK packet with a wrong seqno is sent as a result.

This issue was addressed in #2877 by resetting m_iLargestSeq to SRT_SEQNO_NONE if the list becomes empty.
@gou4shi1 later shared his concerns this may reintroduce the initial concurrency issue.

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Feb 20, 2024
@maxsharabayko maxsharabayko added this to the v1.5.4 milestone Feb 20, 2024
@gou4shi1
Copy link
Contributor

gou4shi1 commented Feb 24, 2024

I am afraid I forgot how does the m_iLargestSeq fixes the concurrency issue.

m_iLargestSeq is updated is both insert() and remove(), so for the case in

When processData() received 899264404, it was added into the recv buffer, after recvbuf_acklock was unlocked, the tsbpd thread played 899264404 and dropped 899264403, then processData() immediately insert 899264403 back into loss list, after this, getFirstLostSeq() keep return 899264403

after 899264403 is removed from loss list by tsbpd(), m_iLargestSeq is updated to 899264403, then processData() can not add back 899264403 into loss list later.

@gou4shi1
Copy link
Contributor

With PR #2877, after 899264403 is removed from loss list by tsbpd(), the loss list is empty, m_iLargestSeq is updated to -1, then processData() can add back 899264403 into loss list later.
Thus I think PR #2877 is not a correct fix.

@gou4shi1
Copy link
Contributor

gou4shi1 commented Feb 24, 2024

We can update m_iLargestSeq in some places (e.g. ack) to fix the m_iSeqNoTH issue in #2873.
@kura979 What do you think?

@gou4shi1
Copy link
Contributor

A similar issue is #1895, caused by seqno wrap around.

@kura979
Copy link
Contributor

kura979 commented Feb 26, 2024

@gou4shi1

We can update m_iLargestSeq in some places (e.g. ack) to fix the m_iSeqNoTH issue in #2873.

It's OK if it means to keep updating m_iLargestSeq so that the distance is less than m_iSeqNoTH.

@maxsharabayko
Copy link
Collaborator Author

PR #2931 temporary resolves this until #2527 is merged.

@maxsharabayko maxsharabayko modified the milestones: v1.5.4, v1.6.0 Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

3 participants