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

[2.x] BoundedReplayBuffer temporarily leaks memory #6475

Open
nhaarman opened this issue May 7, 2019 · 4 comments

Comments

Projects
None yet
2 participants
@nhaarman
Copy link

commented May 7, 2019

When using Observable.replay(N), the BoundedReplayBuffer keeps up to N+1 items in its buffer. When using replay(1) for example, it keeps a reference to the most recent item, but also the previous, stale item.

Take for example this trivial snippet of code that provides an available Android View as a stream:

val eventsSubject = BehaviorSubject.create<Event<View>>()
val view: Observable<Option<View>> = eventsSubject
    .map { event ->
        when(event) {
            is Attached<View> -> Option.just(event.view)
            is Detached -> Option.empty()
        }
    }
    .replay(1).refCount()

The replay(1) call suggests a single value is cached, but the implementation keeps a strong reference to the previous item as well.
Since this happens as a hidden side effect and rather counter intuitively, it is easy to accidentally leak memory -- even when the client code seems to be careful about it. Especially with Android Views referencing Activity contexts this is problematic.


#5282 proposed a fix for this at the cost of an extra Node allocation, which turned out to be unwanted.
The proposed alternative there refers to RxJava2Extensions#cacheLast, but this only emits the very last item, not intermediates.

@akarnokd

This comment has been minimized.

Copy link
Member

commented May 7, 2019

This is a known limitation of the standard replay. Either you can use a ReplaySubject that exposes a cleanupBuffer() method or write a custom replay that does what #5282 proposed. The third option is to not cache things that are lifecycle sensitive.

@nhaarman

This comment has been minimized.

Copy link
Author

commented May 7, 2019

Right, those options seem valid workarounds for now.

I suppose the fact that this happens somewhat surprisingly and hidden in the implementation details makes it feel like it's a bug. Even when being careful and supposedly 'clearing the buffer' by propagating an empty object it still results in leakage. Without knowing that this happens it's easy to accidentally run into this problem and possibly never find out.

@akarnokd

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Item lifecycle is not part of the Reactive Streams model, thus the operator only guarantees one gets up to a number of items when subscribing to cold replay. People have been using LeakCanary to detect such cases and most likely reworked their flow to not use replay.

@nhaarman

This comment has been minimized.

Copy link
Author

commented May 7, 2019

Seems absolutely fair 👍 The pattern above just really 'works well' to go with a 'streams all the way' flow, though I understand this may not be RxJava's responsibility.

nhaarman added a commit to nhaarman/acorn that referenced this issue May 8, 2019

Don't use replay(1) to cache View references
The `replay` operator internally caches N+1 elements instead of just
N, meaning it will keep a strong reference to the previous emitted
element. When this element references an Activity, leaks occur.

See ReactiveX/RxJava#6475.

nhaarman added a commit to nhaarman/acorn that referenced this issue May 8, 2019

Don't use replay(1) to cache View references
The `replay` operator internally caches N+1 elements instead of just
N, meaning it will keep a strong reference to the previous emitted
element. When this element references an Activity, leaks occur.

See ReactiveX/RxJava#6475.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.