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

Fix memory leak in bounded ReplaySubject due to retaining the node index #1866

Merged
merged 1 commit into from
Nov 15, 2014

Conversation

akarnokd
Copy link
Member

indefinitely once the Subscriber caught up.

For issue #1865.

@benjchristensen
Copy link
Member

This test failed both times:

rx.internal.operators.OperatorMergeDelayErrorTest > testErrorInParentObservableDelayed FAILED
    org.mockito.exceptions.verification.TooLittleActualInvocations: 
    stringObserver.onNext("hello");
    Wanted 2 times:
    -> at rx.internal.operators.OperatorMergeDelayErrorTest.testErrorInParentObservableDelayed(OperatorMergeDelayErrorTest.java:516)
    But was 1 time:
    -> at rx.observers.TestObserver.onNext(TestObserver.java:78)
        at rx.internal.operators.OperatorMergeDelayErrorTest.testErrorInParentObservableDelayed(OperatorMergeDelayErrorTest.java:516)

No idea why or if it's related, but I'd like to understand before merging this.

@benjchristensen benjchristensen added this to the 1.0 milestone Nov 12, 2014
@akarnokd
Copy link
Member Author

I don't think MergeDelayError uses ReplaySubject.

I don't fully understand the OperatorMerge, but it contains a lot of mutable variables and synchronized blocks and I'm not sure but I found some anomalies in:

https://github.com/ReactiveX/RxJava/blob/1.x/src/main/java/rx/internal/operators/OperatorMerge.java#L505

It synchronizes on the MergeProducer's this but reads the MergeSubscriber's wip field which generally is accessed through the MergeSubscriber's this. Same seems to be true for the scalarValueQueue.

@benjchristensen
Copy link
Member

I've tested things locally and it's working for me.

@benjchristensen
Copy link
Member

I opened an issue to track the issue you brought up regarding merge: #1885

benjchristensen added a commit that referenced this pull request Nov 15, 2014
Fix memory leak in bounded ReplaySubject due to retaining the node index
@benjchristensen benjchristensen merged commit d6cbf59 into ReactiveX:1.x Nov 15, 2014
@akarnokd akarnokd deleted the ReplaySubjectMemoryLeakFix branch November 17, 2014 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants