Fix leak linked to event listeners on quality change - #1781
Merged
Conversation
peaBerberian
added
the
Priority: 0 (Very high)
This was referenced Jan 19, 2026
peaBerberian
force-pushed
the
fix/apap-stream-listener-leak
branch
3 times, most recently
from
January 19, 2026 13:26
40be01f to
423a02a
Compare
peaBerberian
force-pushed
the
fix/apap-stream-listener-leak
branch
2 times, most recently
from
January 19, 2026 13:54
ef53a24 to
405b338
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
KunXi-Fox
reviewed
Jan 21, 2026
| for (const rep of updated.removedRepresentations) { | ||
| if (rep === representation.id) { | ||
| if (fnCancelSignal.isCancelled()) { | ||
| if (terminatingCanceller.isUsed()) { |
Contributor
There was a problem hiding this comment.
TBH, I'm not so sure how this change solve the issues, the other changes seems more like renames.
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
Collaborator
Author
|
That second SegmentQueue linked leak seen by @KunXi-Fox seem to have an observed effect: https://github.com/canalplus/rx-player/actions/runs/21204957791/job/60999139813?pr=1781#step:7:60 (~1.7MB after now 1000 quality switches as opposed to the ~4MB we've been seeing generally after 500 switches) On (new) "reload" quality switching memory-tests we even go from ~3.7MB to reliably a few negative kB for some reason that I didn't look at yet (meaning: using a little less memory than when starting)! Anyway results from our memory tests are still currently fuzzy, I'm trying to extract more value from them. |
The #1778 and #1779 issues / PR noticed a leak that seem to arise when multiple quality switches happen. I'm still unsure of the severity (looking at it what this fixes seems very minimal, and we did not notice this yet on production at Canal+ including on low-memory devices for what seems to be a change that has been here for 2 years - but external contributors actually did notice a leak so maybe a set of conditions amplify the issue), but looking closely at the code in question, there does seem to be an improper event listener clean-up on a quality switch. The issue is rooted in the complexity behind how quality switch happen: - depending on heuristics, we may either perform an "urgent" quality switch (where we directly cancel the requests linked to the older quality) or a non-urgent one (where we will wait for the current requests to finish and only after load the new quality). - If non-urgent, we want to still do the requests for the new quality as soon as we can, thus we parallelize it with the pushing operations of the segments we just loaded from the previous quality. Thus when a "non-urgent" quality switch happen, there might be a short time where several quality-linked modules are running at the same time (the old one to push segments, the new one to load them), whereas at first glance they seemed conflicting (one loads and push one quality, the other loads and push another quality of the same thing). This lead to an awkward architecture where the clean-up process of those modules is subtly different than in other RxPlayer modules - this one has actually 2 means to terminate: - its `terminate` parameter, kind of like a SIGTERM: just finish what you're doing (e.g. finish loading segments and/or pushing them then stop). Once the `RepresentationStream` (the module in question) has finished loading segments, it sends a `terminating` event - but it might still be pushing segments. It however has no event to indicate that segments have been pushed, for now. - its `cancelSignal` parameter, more akin to a SIGKILL: terminate everything now without delay. This one is e.g. triggered when stopping the content, changing the track etc. The leaking event listener was wrongly linked to that "SIGKILL" signal, even if it was intended to be cleaned up when the module is not needed anymore. When the module was only "SIGTERMed", it was not cleaned up. --- I chose to clean it up not right when "SIGTERMed", but when the module itself anounced that it is "terminating" (it is done loading and is now pushing segments). I found it to be more appropriate for the logic in question and a corresponding `CancellationSignal` was already used for other similar logic linked to the same lifetime.
peaBerberian
force-pushed
the
fix/apap-stream-listener-leak
branch
from
January 23, 2026 11:07
d90ca0a to
b20b8bf
Compare
|
✅ Automated performance checks have passed on commit DetailsPerformance tests 1st run outputNo significative change in performance for tests:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The #1778 and #1779 issues / PR noticed a leak that seem to arise when multiple quality switches happen.
I'm still unsure of the severity (looking at it what this fixes seems very minimal, and we did not notice this yet on production at Canal+ including on low-memory devices for what seems to be a change that has been here for 2 years - but external contributors actually did notice a leak so maybe a set of conditions amplify the issue), but looking closely at the code in question, there does seem to be an improper event listener clean-up on a quality switch.
The issue is rooted in the complexity behind how quality switches happen:
depending on heuristics, we may either perform an "urgent" quality switch (where we directly cancel the requests linked to the older quality) or a non-urgent one (where we will wait for the current requests to finish and only after load the new quality).
If non-urgent, we want to still do the requests for the new quality as soon as we can, thus once the older requests are finished we parallelize its pushing operations with the new requests.
Thus when a "non-urgent" quality switch happen, there might be a short time where several quality-linked modules are running at the same time (the old one to push older segments, the new one to load newer ones), whereas at first glance they could seem conflicting (one loads and push one quality, the other loads and push another quality of the same thing).
This lead to an awkward architecture where the clean-up process of those modules is subtly different than in other RxPlayer modules - this one has actually 2 means to terminate:
its
terminateparameter, kind of like a SIGTERM: just finish what you're doing (e.g. finish loading segments and/or pushing them then stop).Once the
RepresentationStream(the module in question) has finished loading segments, it sends aterminatingevent - but it might still be pushing segments.It however has no event to indicate that segments have been pushed, for now.
its
cancelSignalparameter, more akin to a SIGKILL: terminate everything now without delay.This one is e.g. triggered when stopping the content, changing the track etc.
The leaking event listener was wrongly linked to that "SIGKILL" signal, even if it was intended to be cleaned up when the module is not needed anymore. When the module was only "SIGTERMed", it was not cleaned up.
I chose to clean it up not right when "SIGTERMed", but when the module itself anounced that it is "terminating" (it is done loading and is now pushing segments).
I found it to be more appropriate for the logic in question and a corresponding
CancellationSignalwas already used for other similar logic linked to the same lifetime.