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

New integration: AndroidX Lifecycle #760

Closed

Conversation

Projects
None yet
@LouisCAD
Copy link
Contributor

commented Oct 25, 2018

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.

@LouisCAD LouisCAD changed the base branch from master to develop Oct 25, 2018

@qwwdfsad qwwdfsad force-pushed the Kotlin:develop branch 2 times, most recently from 69dc390 to eaf9b7c Oct 25, 2018

}
}

private val lifecycleJobs = ConcurrentHashMap<Lifecycle, Job>()

This comment has been minimized.

Copy link
@gildor

gildor Oct 26, 2018

Contributor

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

This comment has been minimized.

Copy link
@LouisCAD

LouisCAD Oct 26, 2018

Author Contributor

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 :)

This comment has been minimized.

Copy link
@gildor

gildor Oct 29, 2018

Contributor

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.

This comment has been minimized.

Copy link
@LouisCAD

LouisCAD Oct 31, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@LouisCAD

LouisCAD Nov 2, 2018

Author Contributor

@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.

This comment has been minimized.

Copy link
@qwwdfsad

qwwdfsad Nov 6, 2018

Member

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.

This comment has been minimized.

Copy link
@qwwdfsad

qwwdfsad Nov 6, 2018

Member

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

This comment has been minimized.

Copy link
@LouisCAD

LouisCAD Nov 7, 2018

Author Contributor

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.

Show resolved Hide resolved integration/kotlinx-coroutines-androidx-lifecycle/src/Lifecycle.kt Outdated
@lfielke

This comment has been minimized.

Copy link

commented Oct 31, 2018

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

This comment has been minimized.

Copy link

commented Oct 31, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

@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

This comment has been minimized.

Copy link

commented Oct 31, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

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

LouisCAD added some commits Oct 18, 2018

New integration: AndroidX Lifecycle
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
Move AndroidX lifecycle coroutineScope extensions
…to LifecycleDefaultScopes.kt

That is to prevent class initialization in case they are not used.
Add cached into the jobs and scopes cache maps
This makes the intent more explicit
Replace ConcurrentHashMap by mutableMapOf
Because the maps are just a best effort cache, we don't need the
concurrency safety that ConcurrentHashMap offers.

If the scopes or jobs are accessed concurrently, there may be multiple
created instances, but eventually, an attempt to remove them from the
caching map will always be performed when the lifecycle is destroyed.
Add Lifecycle observer only on next event loop
This allows to support use cases like creating a scope active while
STARTED from an Activity onStart() method.

@LouisCAD LouisCAD force-pushed the LouisCAD:integration-androidx-lifecycle branch from 7f063c6 to a4a6621 Nov 5, 2018

@objcode
Copy link

left a comment

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?

Show resolved Hide resolved ...tion/kotlinx-coroutines-androidx-lifecycle/src/LifecycleDefaultScopes.kt
Show resolved Hide resolved ...tion/kotlinx-coroutines-androidx-lifecycle/src/LifecycleDefaultScopes.kt
Show resolved Hide resolved ...tion/kotlinx-coroutines-androidx-lifecycle/src/LifecycleDefaultScopes.kt
* soon as this [Lifecycle] [currentState][Lifecycle.getCurrentState] is no longer
* [at least][Lifecycle.State.isAtLeast] the passed [activeWhile] state.
*
* **Beware**: if the current state is lower than the passed [activeWhile] state, you'll get an

This comment has been minimized.

Copy link
@objcode

objcode Nov 6, 2018

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?

This comment has been minimized.

Copy link
@LouisCAD

LouisCAD Nov 6, 2018

Author Contributor

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?

This comment has been minimized.

Copy link
@LouisCAD

LouisCAD Nov 6, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@LouisCAD

LouisCAD Nov 6, 2018

Author Contributor

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

This comment has been minimized.

Copy link

commented Nov 6, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

@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

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

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

This comment has been minimized.

Copy link

commented Nov 7, 2018

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.

Fix possible concurrency issues
Replacing mutableMapOf() with ConcurrentHashMap().
Regular HashMap could lead to an infinite loop in some cases where the
map would be accessed concurrently.

@qwwdfsad qwwdfsad force-pushed the Kotlin:develop branch from 630d2da to aab405b Nov 12, 2018

@qwwdfsad qwwdfsad force-pushed the Kotlin:develop branch 2 times, most recently from bc68d63 to 3179683 Nov 12, 2018

@LouisCAD

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

@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

This comment has been minimized.

Copy link

commented Dec 4, 2018

@gmale

This comment has been minimized.

Copy link

commented Dec 27, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

@LouisCAD

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

@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.

@LouisCAD LouisCAD referenced this pull request Dec 28, 2018

Merged

Version 3.0.0-alpha02 #155

3 of 3 tasks complete
@ashdavies

This comment has been minimized.

@svasilinets

This comment has been minimized.

Copy link

commented Jan 22, 2019

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.