Memory leak using "LifecycleExt.kt"

[ultraon]

Hello Arkadiy, thank you very much for such a great library!

It seems to me there is a memory leak in this file: https://github.com/arkivanov/MVIKotlin/blob/bad123550ff7e451708280efbc4394ffdc347107/mvikotlin/src/commonMain/kotlin/com/arkivanov/mvikotlin/core/lifecycle/LifecycleExt.kt#L49

Probably for all these methods: Lifecycle.doOnResumePause, Lifecycle.doOnPause, Lifecycle.doOnStartStop, Lifecycle.doOnStop.

As soon as an anonymous callback instance is created, there is no chance to remove this object until onDestroy event. And if someone uses these methods in onStart then on each call a new callback will be added to LifecycleRegistry.

As a variant to fix that, I can suggest returning Disposable to let a client to remove/unsubscribe from LifecycleRegistry in this way:

inline fun Lifecycle.doOnResumePause(crossinline onResume: () -> Unit, crossinline onPause: () -> Unit): Disposable {
    val callbacks = object : DefaultLifecycleCallbacks {
        override fun onResume() {
            onResume.invoke()
        }

        override fun onPause() {
            onPause.invoke()
        }
    }
    subscribe(callbacks)
    return CancellableDisposable { unsubscribe(callbacks) }
}

// Then from the client side we can call `disposable?.dispose()` to remove added callback.

At the same time there is no such issue with Lifecycle.doOnDestroy and Lifecycle.doOnCreateDestroy (at least on Android platform) because Android LifecycleRegistry removes a callback automatically after onDestroy event.

@arkivanov does it make sense?

[arkivanov]

Hi @ultraon and thanks for the feedback!

All those extensions were designed to be called only once for the lifetime, and so calling them from onStart on onResume is currently considered incorrect. But you are right, calling doOnStop from onStart or doOnPause from onResume actually makes sense.

What do you think about the following API?

inline fun Lifecycle.doOnStop(isOneOff: Boolean = false, crossinline onStop: () -> Unit) {
    // ...
}

If the isOneOff flag is set to true then the callback will be removed automatically. Maybe you can find a better name for the flag?

[ultraon]

Why do you think returning Disposable (or you can name it in another way) is worse? It is more flexible and controllable by a client side.

I can understand your intention in terms of KMM and extensions e.g. Reactive/Suspend, in this case I would agree with your suggestion, regarding naming I think you may also consider oneShot, oneTime (this term is used by Android WorkManager lib - OneTimeWorkRequest), singleCall, singleEvent.

[arkivanov]

My intention here is to simplify as much as possible most common use cases. For advanced use cases there are main Lifecycle.subscribe and Lifecycle.unsubscribe methods. So you can either use it directly or add your own extensions.

[ultraon]

Yes, you are right, thanks for the hint!

[arkivanov]

Alright, I will add the argument, thanks!

[arkivanov]

Will be delivered in version 2.1.0

[arkivanov]

Fixed via #259