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

[core] make sure data to be inserted into CRcvLossList are monotonic #2195

Merged

Conversation

gou4shi1
Copy link
Contributor

@gou4shi1 gou4shi1 commented Dec 1, 2021

fix #2192 (comment)
Previously, these is a comment saying that Data to be inserted must be larger than all those in the list, guaranteed by the UDT receive, let's guarantee this by the internal check.

@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior labels Dec 1, 2021
@maxsharabayko maxsharabayko added this to the v1.4.5 milestone Dec 1, 2021
@gou4shi1
Copy link
Contributor Author

gou4shi1 commented Dec 1, 2021

The result of this PR:

20:15:15.522705/SRT:RcvQ:w1 D:SRT.qr: @812622476: processData: m_pRcvLossList->insert(1571759771 1571759771)
20:15:15.522719/SRT:TsbPd D:SRT.qr: @812622476: dropFromLossLists(): m_pRcvLossList->remove(1571759766, 1571759771)
20:15:15.522753/SRT:RcvQ:w1*E:SRT.qr: RCV-LOSS/insert: IPE: (1571759771,1571759771) to be inserted too small: m_iLargestSeq=1571759771, m_iLength=0, m_iHead=-1, m_iTail=3 -- REJECTING

This log was printed frequently, maybe we should turn down it's level.

@codecov-commenter

This comment has been minimized.

@maxsharabayko
Copy link
Collaborator

@gou4shi1 thank you for this elegant solution!

If the warning message is printed too often, then in addition to this fix, it probably makes sense to find a way to properly synchronize changes to the buffer and loss list. Decreasing log message severity instead will just hide the problem. Will take some time to think. 🤔

Reviewer TODO

  • Check this PR conflict neither with packet filter, nor with the packet reorder tolerance logic.

@gou4shi1
Copy link
Contributor Author

gou4shi1 commented Dec 2, 2021

If the warning message is printed too often, then in addition to this fix, it probably makes sense to find a way to properly synchronize changes to the buffer and loss list.

If those debug logs are disabled, this warning message will not be printed frequently, otherwise, #2192 would be very easy to reproduce :)
During debugging, I think CRcvLossList deserves more sanity checks, I will submit more PRs to add checks with __builtin_expect() and unit tests.

nor with the packet reorder tolerance logic.

I have checked the logic of packet reorder tolerance, seq that was dropped by tsbpd may still be inserted into m_FreshLoss, which may result in some useless lossreports, but it should be fine since ack will be updated correctly.

@gou4shi1 gou4shi1 changed the title [core] make sure data to be inserted into CRcvLossList is monotonic [core] make sure data to be inserted into CRcvLossList are monotonic Dec 2, 2021
@maxsharabayko maxsharabayko merged commit c9a8db7 into Haivision:master Dec 3, 2021
@gou4shi1 gou4shi1 deleted the make-sure-losslist-monotonic branch December 3, 2021 14:51
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

Successfully merging this pull request may close these issues.

[BUG] infinitely loop at CRcvLossList::remove()
3 participants