Skip to content

[core] Fixed missing DROPREQ for LOSSREPORT that partially predates ACK. #2498

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 6, 2022
Merged

[core] Fixed missing DROPREQ for LOSSREPORT that partially predates ACK. #2498

merged 3 commits into from
Dec 6, 2022

Conversation

gou4shi1
Copy link
Contributor

@gou4shi1 gou4shi1 commented Oct 20, 2022 •
edited
Loading

Fixed a bug occurred in group in-order message mode.
I only enabled few heavy logs in production env:

13:16:28.613875/SRT:RcvQ:w3 D:SRT.sm: generateSocketID: : @540801042
13:16:28.614177/SRT:RcvQ:w3 D:SRT.gm: @540801042:synchronizeWithGroup: ST=13:14:17.945509367 [STDY] -> 13:05:18.457541056 [STDY] PST=13:14:17.856090360 [STDY] -> 13:05:18.429949816 [STDY]
13:16:28.614211/SRT:RcvQ:w3 D:SRT.gm: @540801042:AFTER HS: (GROUP, but no TSBPD set)
13:16:28.614251/SRT:RcvQ:w3 D:SRT.gm: applyGroupSequences: @540801042 gets seq from @540801166 rcv %1030134429 snd %1030134390 as GROUPWISE snd-seq
13:16:28.614268/SRT:RcvQ:w3 D:SRT.gm: @540801042:synchronizeWithGroup: DERIVED ISN: RCV=%1030134430 -> %1030134429 (shift by -1) SND=%1030134430 -> %1030134390 (shift by -40)
13:18:55.824817/SRT:RcvQ:w3 D:SRT.gm: applyGroupSequences: @540800754 gets seq from @540801042 rcv %1030134429 snd %1030134390 as GROUPWISE snd-seq
13:25:55.170386/SRT:RcvQ:w3 D:SRT.gm: applyGroupSequences: @540800180 gets seq from @540801042 rcv %1030134429 snd %1030134390 as GROUPWISE snd-seq

Note @540801042:synchronizeWithGroup: DERIVED ISN: RCV=%1030134430 -> %1030134429 (shift by -1).
-> sender sent 1030134428, receiver member A received 1030134428, acked 1030134429
-> sender sent 1030134429, receiver member A not yet received 1030134429
-> receiver member B was added into the group, rcv_isn was set to 1030134430 from the sender handshake, then immediately set to 1030134429 by synchronizeWithGroup()
-> receiver member A received 1030134429, acked 1030134430
-> receiver member A was removed from the group
branch 1:
-> sender sent 1030134430, receiver member B received, reported lossreport [1030134429]
-> sender received lossreport, but 1030134429 was predated m_iSndLastAck (1030134430), ignored
-> receiver member B was stuck at 1030134429 forever (no TLPKTDROP)
branch 2:
-> sender sent 1030134430-1030134431, receiver member B received 1030134431, reported lossreport [1030134429-1030134430]
-> sender received lossreport, but only 1030134430 was added into the send loss list
-> receiver member B was stuck at 1030134429 forever (no TLPKTDROP)

Previously, only if LO < HI < m_iSndLastAck, a DROPREQ will be sent.
This PR fixed these cases: LO < m_iSndLastAck <= HI or LO == HI < m_iSndLastAck.

@gou4shi1 gou4shi1 force-pushed the fix-processCtrlLossReport branch 2 times, most recently from 33eb42c to 69631ea Compare October 20, 2022 06:36
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2022 •
edited
Loading

Codecov Report

Merging #2498 (4f73352) into master (fdb9389) will increase coverage by 0.22%.
The diff coverage is 34.09%.

@@            Coverage Diff             @@
##           master    #2498      +/-   ##
==========================================
+ Coverage   65.91%   66.13%   +0.22%     
==========================================
  Files         100      100              
  Lines       19822    19801      -21     
==========================================
+ Hits        13065    13096      +31     
+ Misses       6757     6705      -52
Impacted Files Coverage Δ
srtcore/logging.h 98.21% <ø> (+3.76%) ⬆️
srtcore/core.cpp 60.65% <31.70%> (+0.75%) ⬆️
srtcore/api.cpp 53.28% <66.66%> (ø)
srtcore/cache.h 80.00% <0.00%> (-3.08%) ⬇️
srtcore/common.cpp 37.32% <0.00%> (-0.93%) ⬇️
srtcore/queue.cpp 81.00% <0.00%> (-0.30%) ⬇️
srtcore/congctl.cpp 82.38% <0.00%> (+1.55%) ⬆️
srtcore/list.cpp 79.15% <0.00%> (+2.90%) ⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gou4shi1 gou4shi1 force-pushed the fix-processCtrlLossReport branch from 69631ea to fcb90c0 Compare October 20, 2022 06:40
@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Oct 26, 2022
@maxsharabayko maxsharabayko added this to the v1.6.0 milestone Oct 26, 2022
@maxsharabayko maxsharabayko self-requested a review November 8, 2022 14:08
@gou4shi1
Copy link
Contributor Author

gou4shi1 commented Dec 2, 2022

any comment?

ethouris
ethouris reviewed Dec 5, 2022
View reviewed changes
ethouris
ethouris reviewed Dec 5, 2022
View reviewed changes
ethouris
ethouris reviewed Dec 5, 2022
View reviewed changes
maxsharabayko
maxsharabayko reviewed Dec 5, 2022
View reviewed changes
@gou4shi1 gou4shi1 requested review from maxsharabayko and ethouris and removed request for maxsharabayko and ethouris December 5, 2022 17:25
@maxsharabayko maxsharabayko merged commit 78dd987 into Haivision:master Dec 6, 2022
@gou4shi1 gou4shi1 deleted the fix-processCtrlLossReport branch December 6, 2022 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethouris ethouris ethouris left review comments

@maxsharabayko maxsharabayko Awaiting requested review from maxsharabayko

Assignees
No one assigned
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Milestone
v1.5.2
Development

Successfully merging this pull request may close these issues.
4 participants
@gou4shi1 @codecov-commenter @ethouris @maxsharabayko