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.

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

Remove RxJS from PlaybackObserver to fix potential leak #1135

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

peaBerberian
Copy link
Collaborator

While testing the future v4.0.0, I noticed a memory leak which way well also affect previous RxPlayer versions.

The leak was that the PlaybackObserver may be still emitting even after playback has been stopped. The actual impacts are non-removed DOM event listeners, unnecessary logic running every seconds or so, and noisy logs

After a lot of console.log-based investigation (as RxJS make the call stack inexploitable as well as step-by-step breakpoints), it seems that the issue is either due to a RxJS bug or of a poor comprehension I have of the shareReplay operator (I would go for the former, but I'm biased!).

Basically, the leaking Observable (the one that is still running despite not being needed anymore) did not have any subscriber left. It had a shareReplay associated to it, with a bufferSize attribute set to 1 and much more importantly a refCount attribute set to true.

The way I understand the refCount option is that the Observable will be shared (it is a shareReplay after all) until no subscriber are left (the reference counter drops to 0) at which point the Observable will be unsubscribed from. This is also what I understand from the code documentation here:
https://github.com/ReactiveX/rxjs/blob/47fa8d555754b18887baf15e22eb3dd91bf8bfea/src/internal/operators/shareReplay.ts#L41-L43

Yet it didn't seem to be the case. In my situation, no subscriber seemed to be left yet the Observable was still performing its side-effects, such as event listening and logs.
As no subscriber was left, nothing else happened, so the damage was limited.
Moreover, removing the shareReplay operator seemed to properly unsubscribe the Observable on which it was applied, which may be another proof that no subscriber was left (though it may not be by itself, but I can assure you that it was the case).


Because it is complex to set-up reproduction steps, because I already found an open issue on the shareReplay operator and because we want to stop using RxJS anyway altogether in the long term, I preferred spending my time today rewriting the PlaybackObserver so it does not depend on a ReplaySubject and even on RxJS.

This was much easier than what I would have thought.
At least two subtle differences exists in behavior with the previous PlaybackObserver though:

  1. Previously the PlaybackObserver started emitting its first event on the first subscription, and (theoretically) restarted emitting if re-subscribed after being totally unsubscribed from anyone.

    Now, the PlaybackObserver is started right away on instanciation.

  2. Previously, the PlaybackObserver (theoretically!!!!!) stopped emitting when all subscribers unsubscribed from it.

    Now, there is an explicit stop method, called by the public_api.ts file (which is the file creating the PlaybackObserver in the first place, so it makes sense).

@peaBerberian peaBerberian added bug This is an RxPlayer issue (unexpected result when comparing to the API) skip-performance-checks Performance tests are not run on this PR Priority: 0 (Very high) This issue or PR has a very high priority. Efforts should be concentrated on it first. labels Jun 24, 2022
@peaBerberian peaBerberian added this to the 3.28.0 milestone Jun 24, 2022
While testing the future v4.0.0, I noticed a memory leak which way well
also affect previous RxPlayer versions.

The leak was that the `PlaybackObserver` may be still subscribed even
playback has been stopped.

After a lot of console.log-based investigation (as RxJS make the call
stack inexploitable as well as step-by-step breakpoints), it seems that
the issue is either due to a RxJS bug or of a poor comprehension I have
of the `shareReplay` operator (I would go for the former, but I'm
biased!).

Basically, the leaking Observable (the one that is still running despite
not being needed anymore) did not have any subscriber left. It had a
`shareReplay` associated to it, with a `bufferSize` attribute set to `1`
and much more importantly a `refCount` attribute set to `true`.

The way I understand the refCount option is that the Observable will be
shared (according to `shareReplay` rules) until no subscriber are left
(the reference counter drops to `0`). This is also what I understand
from the code documentation here:
https://github.com/ReactiveX/rxjs/blob/47fa8d555754b18887baf15e22eb3dd91bf8bfea/src/internal/operators/shareReplay.ts#L41-L43

Yet it didn't seem to be the case. In my situation, no subscriber seemed
to be left yet the Observable was still performing its side-effects, such
as event listening and logs.
Moreover, removing the `shareReplay` operator altogether seemed
to properly unsubscribe from the Observable on which it was applied,
which may be another proof that no subscriber was left (though
it may not be by itself, but I can assure you that it was the case).

---

Because it is complex to set-up reproduction steps, because I already
found an open issue on the `shareReplay` operator and because we want
to stop using RxJS anyway altogether in the long term, I preferred
spending my time today rewriting the `PlaybackObserver` so it does not
depend on a `ReplaySubject` and even on RxJS.

This was much easier than what I would have thought.
At least two subtle differences exists in behavior with the previous
`PlaybackObserver`:

  1. Previously the `PlaybackObserver` started emitting its first event
     on the first subscription, and (theoretically) restarted emitting
     if re-subscribed after being totally unsubscribed from anyone.

     Now, the `PlaybackObserver` is started right away on instanciation.

  2. Previously, the `PlaybackObserver` (theoretically!!!!!) stopped
     emitting when all subscribers unsubscribed from it.

     Now, there is an explicit `stop` method, called by the
     `public_api.ts` file (which is the file creating the
     `PlaybackObserver` in the first place, so it makes sense).
@peaBerberian peaBerberian force-pushed the misc/playbackobserver-no-rxjs branch from 1a3d7ea to 43c1e96 Compare June 27, 2022 08:46
@peaBerberian peaBerberian merged commit 88e86f1 into release/v3.28.0 Jun 27, 2022
@peaBerberian peaBerberian deleted the misc/playbackobserver-no-rxjs branch November 17, 2022 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is an RxPlayer issue (unexpected result when comparing to the API) Priority: 0 (Very high) This issue or PR has a very high priority. Efforts should be concentrated on it first. skip-performance-checks Performance tests are not run on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant