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] fix applyGroupSequences #2735
base: master
Are you sure you want to change the base?
Conversation
774212f
to
2b345fa
Compare
|
||
if (was_empty) | ||
{ | ||
gp->syncWithSocket(s->core(), HSD_RESPONDER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix the issue, it should keep previous group-wise sequences even if the group is empty.
{ | ||
if (m_bConnected) // You are the first one, no need to change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix the issue, it should inherit previous group-wise sequences even if the group is not connected (empty).
@@ -263,7 +201,6 @@ CUDTGroup::CUDTGroup(SRT_GROUP_TYPE gtype) | |||
, m_bSynSending(true) | |||
, m_bTsbPd(true) | |||
, m_bTLPktDrop(true) | |||
, m_iTsbPdDelay_us(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not used.
// number will collide with any ISN provided by a socket. | ||
// Also since now every socket will derive this ISN. | ||
m_iLastSchedSeqNo = generateISN(); | ||
resetInitialRxSequence(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To fix the issue, it should keep previous group-wise sequences even if the group is empty.
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #2735 +/- ##
==========================================
- Coverage 67.51% 66.78% -0.74%
==========================================
Files 99 99
Lines 20166 20148 -18
==========================================
- Hits 13616 13456 -160
- Misses 6550 6692 +142
... and 11 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I'm sorry, but I completely don't understand the fix. The idea for If the problem was that the first member was broken and it caused inconsistency between connection sides, maybe it could be done differently: the ISN in case of group could be created already by a group when connecting the first socket (there is a possibility to enforce ISN when connecting) so that every socket will come in with exactly the same ISN, no matter which one succeeds as the first connection and therefore the first group member. |
Just BTW - I found #2444 that mentions a kind of this problem in the description (Problem 2). Might you be able to check if this solves the same problem or is anyhow related? |
The issue
The log of receiver:
The correct recv base seq was
%787865610
, and then a new member@1071584722
was comming while all other members were broken at the same time.@1071584722
didn't inherit the correct recv base seq, but defined it's own%787865661
However, at the sender side, when the peer of
@1071584722
was coming, the peer of@1071584760
was not yet broken coincidently, so@1071584722
succeeded to inherit the correct send base seq%787865610
fromCUDTGroup::m_iLastSchedSeqNo
.As a result, the sender sent from
%787865610
while the receiver received from%787865661
, the first 51 packets triggeredDROPREQ
, so only the following packets succeeded to transmit.The proposed solution 1 (this PR)
CUDTGroup::m_iLastSchedSeqNo
is a good design, because members status can't be consistent all the time in both side, we should store group-wise send-base-seq and recv-base-seq. It's defined by the first connected socket, and is updated by the ACK timer (not by every packet, to reduce overhead).The proposed solution 2
rm this check
so that RESPONDER can just use the
CUDTGROUP::m_iLastSchedSeqNo
asm_iISN
, instead ofw_hs.m_iISN
, then the wholeapplyGroupSequences()
function can be eliminated.