New integration: AndroidX Lifecycle

[LouisCAD]

This PR replaces #726 which replaced #655.

Includes:

Job and CoroutineScope extensions for Lifecycle and LifecycleOwner
Transitive dependency to kotlinx-android artifact
Tests and test supporting code
Documentation
Entry in binary-compatibility-validator build.gradle file

Note that while binary compatibility text file was generated and is included in this PR, the related gradle task was still failing.

Also, the knit warns for this new module regarding dokka documentation and unresolved references. I don't know how to solve these, but the maintainers can edit this PR, and you're free to help.

[gildor]

I still have to feel that it shouldn't be a part of this integration.
This is a very specific optimization that even don't have any benchmarks to prove that this approach is a lot faster in practice.
And of course, it will be slower than just cache scope in any class that wants to use it (because doesn't involve usage of ConcurrentHashMap).

I would recommend to remove it and keep only simple Lifecycle -> CoroutineScope builders

Such extension property as coroutineScope can be a part of a separate library but probably not an API of official integration

[LouisCAD]

I don't think an entry in the ConcurrentHashMap is so expensive, especially when compared to the cost of creating everything associated to the LifecycleOwner (like UI in an Activity).

If you want to cache the scope the most efficient way, you can still implement CoroutineScope and implement it this way: override coroutineContext = lifecycle.coroutineContext to cache without map access, but by default, you don't need to do it at all.

The reason I did it is to have property syntax (where repeated access doesn't mean repeated allocation), and promote lifecycle scoped usage.

I'm wondering if mutableMap, instead of ConcurrenHashMap would still work correctly given the MutableMap is only used for caching, and that removal from the cache doesn't rely on the map itself, but just on the observer. I think it would be safe for this usage. Anyone thinks otherwise?

Also @gildor, with this information about the why, do you still think coroutineScope extension properties should be removed? I personally use it very often. I can make a separate library just for these two extensions on Lifecycle and LifecycleOwner, but that doesn't seem to be a bad API to me, so I think it could be good to have it into this integration by default. Let me know your rationale or what you think about it :)

[gildor]

I don't think an entry in the ConcurrentHashMap is so expensive, especially when compared to the cost of creating everything associated to the LifecycleOwner (like UI in an Activity).

Why do you compare the creation of Activity vs ConcurrentHashMap instead of a comparison of ConcurrentHashMap vs just a field to cache scope?
I think an extension for creating scope from lifecycle is completely fine, but it should create scope only, like createJob() instead of an implicit caching instance of scope to a global static map.
My point that this is performance optimization with unclear performance gain, imagine if you have some component that uses scope created from lifecycle for example 3-5 times.
You have 3 choices:

1. Create a scope, cache to a field of your component
2. Do not cache scope, so will be created 3 scope instances
3. Use this extension, create a scope, cache it to ConcurrentHashMap

the first option requires more code but obviously more performant (less object created, no atomic or synchronized operations).

Then you have a choice between 2 and 3. And this is not obvious for me that such caching on practice (or even on benchmark) really would be more performant.
So this is why I against this solution and believe that kotlinx.coroutines shouldn't encourage usage of this pattern because it's too implicit and doesn't provide clear advantages or performance gain.

[LouisCAD]

Why do you compare the creation of Activity vs ConcurrentHashMap instead of a comparison of ConcurrentHashMap vs just a field to cache scope?

I'm comparing the runtime cost (memory and CPU), with same code amount, to reason about the slight overhead implied by the global caching map.

You can already decide to only use createScope(…) or createJob(…) and cache it in a property that you can name coroutineScope if you want (as members take precedence over extensions), or use it to implement CoroutineScope.

However, the reason why I provide the coroutineScope extension property is to allow you to use a good default (scope on Dispatchers.Main that is cancelled when lifecycle reaches DESTROYED state) without having to write any boilerplate.

If you start only one coroutine on this default scope for your Lifecycle, then you might as well use createScope(…) and keep the global caching map untouched.

Right now, these coroutineScope extension properties and their cache is in the same file and underlying class as the other extensions, but per your recommendation, I'll make it two classes, so if you don't use coroutineScope, then the global caching map is never initialized as its class is never loaded. I think even without this, if you don't use it, proguard removes it when run (e.g. on your release builds).

For the cache, I think mutableMap should be enough because it's just a cache, and it's not a big deal if the returned job or scope is not the one created on first access, as they will all behave the same way and receive the state updates anyway, in the case you get to this point with multithreaded access. I'll just give myself a bit more thinking time to be certain about this.

[LouisCAD]

@gildor I just pushed 4 commits to make use of coroutineScope optional (no longer referenced from createScope, put in a separate file), and replaced ConcurrentHashMap by mutableMapOf as we don't need the concurrency safety features for this caching use case.

Also, I made it clear, by the name, that the maps are for caching.

Please, let me know what you think of the current version.

[qwwdfsad]

and replaced ConcurrentHashMap by mutableMapOf as we don't need the concurrency safety features for this caching use case.

This is an illegal transformation. Now any access to this map may hang indefinitely (yes, it is possible) or throw NPE.

[qwwdfsad]

In general, I agree with @gildor, it is better not to have a global mutable state as part of this integration, especially when it is not necessary. If one cares about allocations, he will just exract scope into the field or provide another base class.
Global state is probably the biggest source of memory leaks

[LouisCAD]

I had a hard time believing this, and then read that because of the loops and hashing operations in HashMap, you can get an infinite loop on an attempt to read/write. Thanks for the information, TIL.

I'll revert my change back to ConcurrentHashMap to make it safe again, even though this may not make it into this repo, neither into AndroidX (as being inside AndroidX will likely allow an alternative implementation that doesn't need to rely on any global cache). This is to leave a clean PR in case anyone wants to integrate this into a project without waiting for any AndroidX built-in support.

[lfielke]

One of the use cases I hoped this would support is a simple way to launch a coroutine from the onStart() method of a Fragment that was scoped to be alive until the Fragment's onStop was called. I could not get this to work in a simple elegant way though.

Calling Lifecycle.createJob(activeWhile=STARTED) from inside onStart returns a cancelled job. This is because the Lifecycle doesn't get advanced to STARTED until after the user code in onStart returns (docs [1], implementation [2]), so the currentState.isAtLeast(activeWhile) check cancels the job straight away.

From a quick look at the code, I think using activeWhile=RESUMED in onResume would have a similar problem.

This was an unexpected gotcha for me.

N.B. The default value of activeWhile = INITIALIZED sidesteps this problem when used in onCreate, because INITIALIZED is the initial state of the Lifecycle, so the isAtLeast check passes and the job still gets correctly cancelled just before onDestroy is called.

[1] https://developer.android.com/reference/android/arch/lifecycle/Lifecycle.State#started
[2] https://android.googlesource.com/platform/frameworks/support/+/android-9.0.0_r12/fragment/src/main/java/androidx/fragment/app/Fragment.java#2471

[lfielke]

After I wrote the above comment I saw the AppCompatActivity example in the README, that shows the kind of usage in onStart I was talking about. I couldn't get that example to run the coroutine either for the same reason I described above (a cancelled job is returned because the Lifecycle doesn't get advanced until after onStart returns).

[LouisCAD]

@lfielke It's true, the startedScope example can't run and will always be cancelled. I added it before I tried it.
I have two options:

1. Removing it
2. Adding an awaitState(…) extension function for Lifecycle as you have in this gist.

Do you think I should add the awaitState(…) extension function to this integration? Maybe you have a better name idea, or is it already good enough?

[lfielke]

I don't think removing the example will prevent people misinterpreting how to use it. I hadn't seen the example when I first tried it and just assumed that code would work based on the method names and how lifecycle-handling libraries in the RxJava world work. This would take away a large amount of appeal of this PR for me.

Option 2 should work, but it would be nice to avoid the overhead of adding a GenericLifecycleObserver which is just going to be removed straight away in the next loop of the Main Looper.

The current API and implementation has the advantage of doing the right thing when used from other LifecycleObservers (as opposed to from the Activity or Fragment). From what I can see it can't leak a job (ie they'll always get cancelled) and the activeWhile parameter is natural to use because it is the same value as the state you get in your LifecycleObserver callback.

I have two other ideas for consideration, a rough outline as I haven't thought through the details yet...

Idea 1. For this use case, the library could automatically determine the correct lifecycle state where cancelling should occur -- it's the current one. So in the onStart example, the current state is CREATED. The job should be cancelled upon the next entry to the CREATED state. This has the nice property of working like https://github.com/uber/AutoDispose where you don't need to pass the state as a parameter.

The downside of this idea is that it's a bit "magic" and would do the wrong thing if called from onDestroy for example (because the job would get cancelled upon getting a CREATED state again, which will never come).

Idea 2. Add/Change an API to be based on Lifecycle.Event instead of State. The function would then be something like:

fun Lifecycle.createJob(activeUntil: Lifecycle.Event = ON_DESTROY): Job

The advantage of this is that it's completely unambiguous when the job will get cancelled.
A disadvantage is that it's still possible to leak jobs (ie they never get cancelled). For example you could call createJob(activeUntil=ON_PAUSE) from the onPause method. The library can't determine that you've "passed" the point when that event is guaranteed to fire, because the Lifecycle is in the same state (STARTED) as it would be in onResume where it does make sense.

Both of these misuses in idea 1 and 2 could be guarded by always cancelling the job in the callback for DESTROYED though (and possibly throwing an exception if it "fell through" to DESTROYED due to misuse). So it might not be a huge problem in practice.

[LouisCAD]

@lfielke Thanks for your thoughtful feedback!

For the started state problem, maybe I can fix this issue with yield(). I'll see how I can implement this nicely.

For your idea 1:
While I think it could be cool to have it automatically pick the corresponding state, I think it is not explicit enough, if at all (which goes against what Kotlin strives to be), and it would be too easy to mess it up.

For your idea 2:
Actually, this was the first design I had when it was only a gist (see history here), but I ended up dismissing it because of the issues you mentioned: possible leaks and possible misuse.
I strongly prefer the current, safer design with state because of this, and also because of how it makes you think in a way that implicitly doesn't allow leaks.

[LouisCAD]

@lfielke With latest commit (7f063c6), createScope(activeWhile = STARTED) now works as expected when called from onStart() in an Activity!

[objcode]

Took a look! Nice PR.

I am not sure cancelling a scope job is a natural match for Android lifecycles. What do you think about a pausable dispatcher instead?

[objcode]

This API surprises me in two ways:

1. If I instantiate this very early (e.g. in field initialization) it will always end up creating a cancelled job which I won't discover until my coroutine doesn't run.
2. If a state is re-enterable (e.g. resumed on Fragment) this will not resume again.

What do you think a lifecycle aware dispatcher to solve both of those cases as well as handle the onCreate situation discussed previously?

I'm not sure this exactly the right API, but it seems likely to work out for these cases. Basically, it'd operate as an event loop that pauses when it's below the expected state. What do you think?

[LouisCAD]

If you want to launch a coroutine at instantiation time with a lifecycle aware scope, you can still use createScope(activeWhile = Lifecycle.State.INITIALIZED).

Do you think I should change the default for the coroutineScope and job properties to be active while initialized instead of active while created?

[LouisCAD]

About your second point: the typical use case there is to launch a coroutine inside the onStart or onResume method that is automatically cancelled when the lifecycle is paused or resumed, so a new coroutine is launched each time the onStart or onResume method is called.

[LouisCAD]

When a Job is cancelled, there's no going back, but you can always start a coroutine later when the conditions are met again, as mentioned in my comment just above.

About pausable dispatcher, I thought about this a while ago, but this doesn't fit well with coroutines design.

You could think that you could pause dispatching, but this could cause more problems, as you may be relying on a dispatching loop to release resources, creating temporary leaks in suspended coroutines that may resume only much later, if ever.

Suspending work based on the lifecycle has to be opt-in at call site.
This is something that is possible with this awaitState(…) extension function for Lifecycle I wrote, and that can work well for the use cases where you want to pause execution until the lifecycle is at least in some state. You can call this before starting expensive work or in a loop tied to the UI component (Activity, Fragment…).

I personally already have use cases for this method, like showing DialogFragments safely.
Do you think I should include this method in this PR?

[svasilinets]

Hey, androidx team here! Thanks for CL, however: we firmly believe that Lifecycles + Coroutines integration is much better fit for lifecycle ktx library than kotlinx-coroutines-android and should be developed in aosp. Reasons for that are quite simple:

• Healthy dependencies order. Coroutines library provides more base (core/low-level) abstractions than androidx.lifecycles, and higher level abstractions should depend on lower-level not otherwise. So in androidx we're going to have more and more things that are integrated with coroutines (workmanager and room and obvious examples), and it will be weird and wrong to bring all of them to coroutine library.
• Androidx team completely agrees that integration with coroutines is very important and we're happy to own and develop this story in all our libraries (including lifecycles). So, for example, ViewModels the integration is already landed in the tip of the tree.

Finally, androidx welcomes contributions as well, there is a guide how to do that.

[LouisCAD]

@svasilinets Here's a quote from the page you linked:

New features to existing libraries if the feature request bug has been approved by an AndroidX team member.
We are not currently accepting new modules.

What should I do?

[qwwdfsad]

Hi @svasilinets, happy to hear that! Could you please help Louis with an initial effort on how to create a proper PR?

I am ready to elaborate if some help with integration is required.

[svasilinets]

What should I?

In general: ktx stuff is usually a small scale you can simply draft a CL and send it. If you have large scale feature, then file a bug and assign to someone who is related to the area (use OWNERS files in frameworks/support and subfolders to look for related people), so you can check if anyone already works on something similar or find out if we're willing or not to take on long-term support of this feature.

In this particular case: our team explored and drafted some approaches on the same topic in parallel with you, I'll create a work in progress CL so we can openly discuss there which approach we'd like to take. (I'll send a link here later). That's said, it doesn't mean you shouldn't send a CL, just saying that we were working on this too and nothing is set yet. If you send a CL add me (sergeyv@) as a reviewer, I'll add all interested parties and give you all needed pointers related to our tooling.

@qwwdfsad
Thanks It would be nice to add you to the review once we have a CL.

[LouisCAD]

@svasilinets Is there any news about the "CL"? (It's not clear what this acronym means BTW)
I plan to wait one more week, then I'll put this in a third party library if there's no progress.

[hrach]

I had to google this: https://stackoverflow.com/questions/25716920/what-does-cl-mean-in-commit-message-what-does-it-stand-for

They are probably waiting on your PR.

[gmale]

I plan to wait one more week, then I'll put this in a third party library if there's no progress.

@LouisCAD where did you land with this? Did you send a CL to androidx? Are you incorporating it into your Splitties library?

[LouisCAD]

I got a timeout I guess. I'll make a module in Splitties right now, and will update here when it is released.

…

On Thu, Dec 27, 2018, 7:49 PM Kevin Gorham ***@***.***> wrote: I plan to wait one more week, then I'll put this in a third party library if there's no progress. @LouisCAD <https://github.com/LouisCAD> where did you land with this? Did you send a CL to androidx? Are you incorporating it into your Splitties <https://github.com/LouisCAD/Splitties/blob/8d1b5224bc1b17c06933f50ec3c0d3faa6d7ebf6/sample/src/main/kotlin/com/louiscad/splittiessample/extensions/coroutines/LifecycleDefaultScopes.kt> library? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#760 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGpvBUIkVXrHDSx9Z0_62kmZEXYk0SITks5u9RYbgaJpZM4X547U> .

[LouisCAD]

@gmale I wanted to deploy a snapshot but artifactory seems to have forgotten my account, so I'll go to sleep, and after this night, I should be able to finish this PR (LouisCAD/Splitties#155), merge it and release it to bintray+jcenter.

[ashdavies]

There has also now been ViewModel.viewModelScope added to androidx.lifecycle 2.1.0-alpha01

https://developer.android.com/jetpack/androidx/androidx-rn#2018-dec-17-lifecycle
https://android.googlesource.com/platform/frameworks/support/+/androidx-master-dev/lifecycle/viewmodel/ktx/src/main/java/androidx/lifecycle/ViewModel.kt

[svasilinets]

@qwwdfsad androidx version is out for review, we'd happy if you can take a look
https://android-review.googlesource.com/c/platform/frameworks/support/+/869365