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

Make inner class static for CompletableOnErrorComplete #7574

Closed
wants to merge 2 commits into from
Closed

Make inner class static for CompletableOnErrorComplete #7574

wants to merge 2 commits into from

Conversation

chintando
Copy link
Contributor

@chintando chintando commented Jun 23, 2023

Thank you for contributing to RxJava. Before pressing the "Create Pull Request" button, please consider the following points:

After taking a heap dump for a Rx chain that was completed, we saw evidence of inner class showing up on the heap dump analysis. It apears that CompletableOnErrorComplete#onError is leaking the predicate

For instance

class Foo {

fun test(): Completable {
Completable.fromCallable {
println("hello world")
}.onErrorComplete { --> leaks after chain completion
true
}

}

}

Verified with kotlin bytecode dump and heap analysis

  • Please give a description about what and why you are contributing, even if it's trivial.

  • Please include the issue list number(s) or other PR numbers in the description if you are contributing in response to those.

  • Please include a reasonable set of unit tests if you contribute new code or change an existing one. If you contribute an operator, (if applicable) please make sure you have tests for working with an empty, just, range of values as well as an error source, with and/or without backpressure and see if unsubscription/cancellation propagates correctly.

Make OnError class static to avoid memory leak
[WIP]Update CompletableOnErrorComplete.java
@akarnokd
Copy link
Member

Your change makes no sense as the OnError loses the reference altogether and causes a compile error.

Usually a leak is caused by your application holding references, for example, in a CompositeDisposable which is not cleaned up. There are workarounds such as onTerminateDetach or using the 4 argument subscribe method that removes itself from a CompositeDisposable upon termination.

@akarnokd akarnokd closed this Jun 23, 2023
@chintando
Copy link
Contributor Author

i believe the OnError should have been taking the predicate as part of constructor, won't the onError hold on to predicate thereby not being able to clean up the predicate object? Just curious

@akarnokd
Copy link
Member

akarnokd commented Jun 23, 2023

Predicates are never cleared in RxJava.

i believe the OnError should have been taking the predicate as part of constructor

Yes, that could be done. If you want, you can resubmit the PR with this change.

@chintando
Copy link
Contributor Author

sure thing will do

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.

None yet

2 participants