Open gitamirp opened 8 months ago
@gitamirp,
@jmillan thanks for looking into it. We are using version 3.12.12. Unfortunately, it happens in a production environment therefore its harder to get detailed logs or STR for it. It rarely happens but I can try to get some more info around it.
You should upgrade to latest release first, 3.12.12 is very old and the issue you are hitting might have been long fixed
Yes please, you're using quite an old version. Upgrade it as soon as you can.
Don't hesitate to reopen the issue if it comes out in the future.
Honestly there is no change since 3.12.12 that could have fix this issue. Let me reopen to not forget. I think we need a fuzzer test for this class.
thanks, fwiw there were no significant changes to RateCalculator.cpp over the last 2 years.
https://github.com/versatica/mediasoup/commits/v3/worker/src/RTC/RateCalculator.cpp
I suspect it might occur when
https://github.com/versatica/mediasoup/blob/v3/worker/src/RTC/RateCalculator.cpp#L27
nowMs - this->newestItemStartTime >= this->itemSizeMs
and
https://github.com/versatica/mediasoup/blob/v3/worker/src/RTC/RateCalculator.cpp#L32
this->newestItemIndex >= this->windowItems
and
this->oldestItemIndex == 0
probably just logging an error and resting the RateCalculator will be more appropriate error handling
probably just logging an error and resting the RateCalculator will be more appropriate error handling
Defensive programming hides real bugs. We don't do that. If something should never happen then it must never happen, otherwise it's a bug that needs to be fixed.
If there was a memory corruption somewhere, side-effects might be unexpected. This needs to be confirmed on latest version first.
probably just logging an error and resting the RateCalculator will be more appropriate error handling
Defensive programming hides real bugs. We don't do that. If something should never happen then it must never happen, otherwise it's a bug that needs to be fixed.
also true
If there was a memory corruption somewhere, side-effects might be unexpected. This needs to be confirmed on latest version first.
I am not sure how fast we can get these hosts on latest release. for now I will just patch it to suit our needs.
I'll let you know if I find more about this condition.
thanks
@gitamirp any news? I assumed you patched version 3.12.12 so it's not a problem for you anymore but perhaps you updated to latest version (without patching it)?
We patched 3.12.12 by using
MS_ASSERT( this->newestItemIndex != this->oldestItemIndex || this->oldestItemIndex == -1 || this->newestItemIndex, "newest index overlaps with the oldest one");
I will let you know if we run into this ASSERT again with this patch. If you are interested we can add some logs when
this->newestItemIndex == this->oldestItemIndex
I believe that in 3.12.12 both newest && oldest can be zero at the same time but I don't have logs to support it.
Yes please. Add those logs and comment here when you get something. We will investigate this next week. Too busy these days.
Sure, we'll do. thanks
worker hits assert in
https://github.com/versatica/mediasoup/blob/v3/worker/src/RTC/RateCalculator.cpp#L37