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.

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

Fix regression (crash) in transport-cc feedback generation #1339

Merged
merged 6 commits into from Feb 20, 2024
Merged

Fix regression (crash) in transport-cc feedback generation #1339

merged 6 commits into from Feb 20, 2024

Commits on Feb 20, 2024

  1. Fix regression (crash) in Transport CC handling 

    - Fixes #1336

### Details

- Regression introduced in PR #1088, which has many issues.
- Basically there are cases in which `this->transportCcFeedbackPacket` is reset so it's a complete new packet and hence its `baseSet` is `false`.
- However in `TransportCongestionControlServer::FillAndSendTransportCcFeedback()` in which we just call `this->transportCcFeedbackPacket.SetBase()` once at the top, but then we enter a loop in which the packet maybe full so it's sent (or other things) and hence we **reset** the packet while in the loop. And then at the top of the loop `this->transportCcFeedbackPacket.AddPacket()` is called so the assertion fails because its `SetBase()` was not called on that fresh packet.
- Also add a missing `break` in a `case` block in that loop.
- Also set proper packet count in all cases in which we reset `this->transportCcFeedbackPacket`.

### TODO

This is not done yet. The issue is mainly identified but I don't know yet how to properly fix it without doing other wrong things. Basically I'm not sure if the whole logic is ok after PR #1088:

- As said before we only set the base of the packet once at the beginning of the loop, and such a `SetBase()` is called with `this->transportCcFeedbackWideSeqNumStart` and a computed `timestamp`.
- However, within the loop below, the packet may become full and hence `SendTransportCcFeedback()` is called, which updates `++this->transportCcFeedbackPacketCount` and `this->transportCcFeedbackWideSeqNumStart`, and **also** resets the packet (so missing base again).
- So in order to fix the crash, we must call `SetBase()` again, but which which values? Because the values given initially at the top of `TransportCongestionControlServer::FillAndSendTransportCcFeedback()` were the value of `this->transportCcFeedbackWideSeqNumStart` at that time and a timestamp. But that `this->transportCcFeedbackWideSeqNumStart` has now changed, so then what?
- Basically every time `this->transportCcFeedbackPacket.reset()` is called (and there are various cases in the whole `TransportCongestionControlServer.cpp` file) **we must** call ``SetBase()` on the fresh packet **with** proper values. And which are those proper values if we are within that loop?
    @ibc
    ibc committed Feb 20, 2024
    Configuration menu
    Copy the full SHA
    0d1dfa7 View commit details
    Browse the repository at this point in the history

  2. Call this->transportCcFeedbackPacket->SetBase() when needed

    @ibc
    ibc committed Feb 20, 2024
    Configuration menu
    Copy the full SHA
    42a0968 View commit details
    Browse the repository at this point in the history

  3. cosmetic

    @ibc
    ibc committed Feb 20, 2024
    Configuration menu
    Copy the full SHA
    d1d1231 View commit details
    Browse the repository at this point in the history

  4. Update CHANGELOG

    @ibc
    ibc committed Feb 20, 2024
    Configuration menu
    Copy the full SHA
    a975137 View commit details
    Browse the repository at this point in the history

  5. fix scenario in which feedback-cc packet is full but cannot be sent

    @ibc
    ibc committed Feb 20, 2024
    Configuration menu
    Copy the full SHA
    b8a411b View commit details
    Browse the repository at this point in the history

  6. Add private TransportCongestionControlServer::ResetTransportCcFeedbac… 

    …k() method
    @ibc
    ibc committed Feb 20, 2024
    Configuration menu
    Copy the full SHA
    77d0df2 View commit details
    Browse the repository at this point in the history