oldergod / android-architecture

MVI architecture Implementation of the ToDo app.
Apache License 2.0
669 stars 70 forks source link

Intent subscription is never disposed #56

Closed steffandroid closed 5 years ago

steffandroid commented 5 years ago

We've been using a variation of this architecture for just over a year at VanMoof, and for the most part it's been working great. However, we encountered an issue with the processIntents() function in each ViewModel:

override fun processIntents(intents: Observable<TasksIntent>) {
  intents.subscribe(intentsSubject)
}

The statesObservable is added to a CompositeDisposable in each fragment and so is disposed correctly, but this isn't propagated by intentsSubject, so a memory leak occurs here. We found this to be particularly problematic when using RxBroadcastReceiver for sending intents because the receiver would never be unregistered, so we would end up with intents being duplicated when re-opening a screen.

We solved this by adding the intents subscription to a CompositeDisposable in each ViewModel and disposing it in onCleared():

override fun processIntents(intents: Observable<TasksIntent>) {
  disposables.add(intents.subscribe(intentsSubject::onNext))
}

override fun onCleared() {
  disposables.dispose()
}
oldergod commented 5 years ago

Thank you for pointing out, I think you're right. Since the ViewModel instance persists config changes, shouldn't it be disposables.clear() instead of dispose() ?

steffandroid commented 5 years ago

onCleared() isn't called when a configuration change occurs, only when the ViewModel is no longer being used and ready to be destroyed, so I think it's safe to call dispose().

oldergod commented 5 years ago

Thank you!