https://kotlinlang.org logo
Join Slack
Powered by
Folks who inject `CoroutineScope`s with semantic m...
# coroutines
u
Folks who inject `CoroutineScope`s with semantic meaning, how do you deal with mismatch against DI?
Copy code
@SingleIn(UserScope::class)
class MyDependency(
   @QualifierFor(AppScope::class) private val scope: CoroutineScope
) {
   fun foo() {
      scope.launch {
         // access other fields
      }
   }
}
since this compiles but is a memory leak
z
Yea this looks smelly. I think you'd want to inject `CoroutineContext`s, not `CoroutineScope`s. The contexts should be used for context other than the `Job`/lifetime.
In the codebase I work in, we inject contexts for things like main and IO dispatchers, but lifetime/scoping is provided by other mechanisms (which have helpers to create `CoroutineScope`s).
u
But how does that help? I presume you have different contexts, one per semantic scope, no?
Why I'm asking mostly is because of testing. 
runTest { .. }
seems to want to push me in the direction of
Copy code
runTest {
    val x = MyDependency(this)
}
such to get the cancellation out of the box
z
No, the contexts don't contain anything to do with "scope" – they're just Dispatchers, although could potentially contain other context elements, but not Jobs.
So e.g. in some of our code we have stuff like, to adapt your example,
Copy code
@SingleIn(UserScope::class)
class MyDependency(
   @QualifierFor(AppScope::class) private val context: CoroutineContext
) : Scoped {
   fun foo() {
      asCoroutineScope().launch(context) {
         // access other fields
      }
   }
}
Where 
Scoped
is an interface that our system wires up to start/stop callbacks, and has an extension method 
asCoroutineScope()
that creates a 
CoroutineScope
with a job that gets cancelled when the scope is disposed.
u
Okay so if I deobfuscate your idea, it's this inside?
Copy code
class MyDependency(private val dispatcher) {
	private val scope = CoroutineScope(disptacher + SupervisorJob())

	// ...
	
	fun stop() {
		scope.cancel()
	}
}
i.e. a 
private val scope
?
z
basically yea
u
that's actually what I'm doing right now, since I've no idea as how to solve that potential mismatch issue but then I'm getting yelled at by the testing folks, since they want me to take in the `TestScope`from 
runTest
which I mean I could provide a new constructor, and have `@Inject`ed just one of them .. but that's kinda lame
since now at test time with your/my pattern I need to call 
stop
explciitly, which nobody does..
z
Yea it doesn't follow structured concurrency, but in our case we were adapting a much more legacy system to coroutines, so we had to handle that ourselves before anyway
u
if you were to forgo the legacy, what would you do?
z
starting green-field today, i would use a modern framework that has coroutine-friendly scoping built-in, like Slack's 
Circuit
or Square's 
Workflow
or just use compose directly, in lieu of circuit
u
do you know how they solve that? it seems to me this is a universal issue, if you want semantic DI scopes
z
if you're using compose, then you'd probably write tests using molecule, which takes a coroutine scope to run the compose runtime. That's the cleanest example
Workflow has an entry point that also takes a coroutine scope to run its runtime in
u
right, but if you want to extend that, like AppScope <- UserScope, you need a coroutine scope thats shorter, no?
z
i'm not sure how helpful it is to continue comparing to compose/workflow if your framework is significantly different, the specific patterns might not be translatable
u
yea probably not, since I'd bet its a codegen/linting solution, since they have custom annotations for DI; and not plain dagger
well my framework doesnt really matter, im just looking for ideas in general as how to solve this, or design this away
z
if you want to make your example play nice with structured concurrency, you could probably just do something like:
Copy code
class MyDependency(private val context: CoroutineContext) {
	private val scope = CoroutineScope(context + SupervisorJob(parent = context[Job]))

	// ...
	
	fun stop() {
		scope.cancel()
	}
}
Then in your test you can pass in a context with a job, but all the coroutines in 
MyDependency
are still scoped to the lifetime of that class via your own mechanism.
And in production you can just inject a 
Dispatcher
or whatever
That's a little weird because it's possible for the coroutine job to be cancelled before your 
stop
method is called, so there's kind of two sources of truth for lifetime.
Maybe a better solution would be to have a test helper that stops scoped objects after the test.
Since that would be a problem that would need to be solved anyway for things that are doing things in 
stop
other than cancelling a coroutine job
u
the 
test helper
, how would that look like? pop in Scoped references into it in 
@Before
?
z
it could look like a lot of things. If you have a testing team that's recommending practices, i would think it would be something they would build.
in production, what calls 
stop()
?
u
when the scope dies .. say UserScope dies on logout
z
do you have an actual framework you're building against, or is this a one-off thing?
right, but what kills it?
something has to actually call 
stop()
u
Scopes.remove(UserScope::class)
like an explicit call to it, last line of 
suspend logout
no "framework", handrolled everything
Scopes
is just a map of dagger components, thats all and on remove, scope gets all its 
Scoped
instances (multibinding) and calls stop on each
z
In your tests, are you using dagger or just instantiating 
MyDependency
directly?
u
both, in "small" tests im instantiating directly in big tests I do have a DI graph thats android-less yea I'm mostly talking about the small tests now
Copy code
class FooSyncer(private val dispatcher) {
	private val scope = CoroutineScope(disptacher + SupervisorJob())

	fun sync() {
		scope.launch {
			doSync()
		}
	}

	private suspend fun doSync() {
		//
	}

	fun stop() {
		scope.cancel()
	}
}
one such test would be to test 
sync()
here
Copy code
runTest {
    val syncer = FooSyncer(this if it were to take scope)
    syncer.sync()
    advanceUntilIdle()
    assertThat(db).isTheWayIWantIt()
}
z
so you could do something as simple as
Copy code
inline fun <T: Scoped> T.use(block: (T) -> Unit) {
  try {
    block()
  } finally {
    stop()
  }
}
and then write your tests like
Copy code
@Test fun thingsWork() {
  MyDependency(…).use { dependency ->
    // test code
  }
}
of course throwing 
runTest
in there when you need coroutines
u
and then harass people/lint to force usage? 😄
z
Or something like
Copy code
fun CoroutineScope.stopOnCompletion(scoped: Scoped) {
  coroutineContext.job.invokeOnCompletion { scoped.stop() }
}
and write tests like
Copy code
@Test fun thingsWork() = runTest {
  val dependency = MyDependency(…)
  backgroundScope.stopOnCompletion(dependency)
}
I think that's a little more gross though, since it is more coupled to coroutines specifically
If you're not using a framework that forces everything to be cleaned up, then you'll have to enforce some convention no matter what
Even if your solution is just passing the TestScope to MyDependency, you'd need to enforce that too
u
how could the coroutinescope ctor pattern be misused? .. do you mean against something like
Copy code
runTest {
   val x = MyDependency(GlobalScope)
}
this?
btw with the "frameworks", don't they allow for DI frameworks like dagger, to be used directly??
z
sure, or 
CoroutineScope()
, or 
EmptyCoroutineContext
yea, the docs for Circuit have an example specifically using dagger 
AssistedInject
u
yea good point
z
and workflow supports basically the same thing
u
yea, the docs for Circuit have an example specifically using dagger 
AssistedInject
okay but then notion of scopes is not from dagger, but rather from Circuit?
z
yep
the "scope" of a circuit presenter is the presence of its 
present
function in composition
u
yea, thats probably the only way, to not allow coroutine scope in DI graph
z
and circuit has suspending testing helpers
u
I see, interesting btw at test time, do you substitute every dispatcher for the 
StandardTestDispatcher
? i.e. all the references (io, default, main) point to that single instance , thus making is effecivelly single threaded?
I was thinking about something like keeping io and default real, and just swap main for a singlethreaded executor turned to dispatcher but not sure if it's a good idea obviously I cannot idle on it, but if turbine was used, like in what you pasted
z
I don’t actually know what we use for IO dispatcher in tests that need one. I’m guessing it probably doesn’t matter since we’d fake the io calls anyway.
u
well its not really about the io but interplay of dispatchers; which is gone if all point to the same one, which has 1 thread afaik
z
Why do you want to run the tests multithreaded?
u
naively it seems to me that it would be better, more real
z
For integration tests, sure.
u
if turbine controls the flow
z
For unit tests, unless you’re specifically testing code that is supposed to be thread safe to ensure it actually is, it would just make the tests unnecessarily complicated
And you’d effectively be testing the synchronization mechanisms already in coroutines (or whatever you’re using), which should have their own tests.
u
˙
Copy code
fun foo(): SomeResult {
	return coroutineScope {
	   val x = async { .. }
	   val y = async { .. }
	   x.await() + y.await()
	}
}
say its something trivial like this isnt there benefit to have the asyncs actually be concurrent at test time?
z
They are always concurrent, no matter what dispatched you’re using.
u
well, are they if its just one thread behind it all?
z
Concurrency has nothing to do with threading
You’re talking about parallelism
u
I know it would not make a difference in that case, but overall the spirit of .. more real, more better? sort of like backend folks do with integration tests i.e. just shoot http requests against instance of the whole app
z
There’s no value to having them run in parallel unless they contain code that actually does something like races access to shared data. And then you’d need to write specialized tests to increase the probability of hitting race conditions.
App code using coroutines shouldn’t care
Unit tests should test individual units, they’re not integration tests
u
hm, then just pass in 
Dispatchers.Unconfined
everywhere?
z
I’d use the test dispatcher probably
u
right but, there is 
UnconfinedTestDispatcher
as well, okay I probably dont know what unconfined actually does
z
And try to avoid unconfined dispatchers for tests in general since they can break things in subtle ways
u
but does 
StandardTestDispatcher
take in the actual test thread or have it's own backing it? that would make sense
do you have use for unconfined at all? I was only using them in tests oops 😄
z
It doesn’t have its own thread, just runs on the thread where you call runTest.
u
then how the hells is it different from unconfined 😄
z
Or where you call advance, if you happened to do that from another thread, which idk why you would but I suppose it’s possible
Read the docs
u
yea sorry .. thanks for all the massive help!
👍🏻 1
s
@Zach Klippenstein (he/him) [MOD] why is it bad to inject CoroutineScopes? In our Android app we have an "application scope" which is a singleton CoroutineScope with a SupervisorJob that is provided via DI. We use it at some places to launch coroutines that should run in the scope of the application, that is their lifecycle should be larger than for example that of a ViewModel.
👍 1
I assume if I inject a singleton "application" CoroutineContext with a SupervisorJob that would be the same result but better?
Would this then be okay? Just creating a new scope where needed?
Copy code
CoroutineScope(appContext).launch { }
l
I think the whole discussion started around the question if you should pass a 
CoroutineScope
to classes because then you could pass an 
ApplicationScope
where actually you should have e.g. 
ViewModelScope
.
The way we do it, so we don't have to think about it, is in ViewModels we only inject the scope for tests:
Copy code
class MyViewModel(
    providedCoroutineScope: CoroutineScope? = null
) {
    private val coroutineScope = providedCoroutineScope ?: viewModelScope
}
For repositories we set a default argument (and also only overwrite the scope for tests):
Copy code
class MyRepository(
    private val coroutineScope: CoroutineScope = ApplicationScope.instance
)
And then replace the scopes in tests with 
backgroundScope
.
But we also don't use any DI framework
We inject everything manually, mostly through default arguments, and provide singletons through `object`s (like above the 
ApplicationScope.instance
)
u
If you inject scope, you are at risk of a mismatch with the classes DI scope, which is then a memoryleak @svenjacobs
l
Would this then be okay? Just creating a new scope where needed?
CoroutineScope(appContext).launch { }
To guarantee structured concurrency you would need to store this CoroutineScope in class, so all methods use common instance. Also you need to cancel it in 
onCleared()
(for ViewModels).
s
Can there be a memory leak if it's a singleton, ensured by DI?
u
whats singleton?
l
A memory leak is e.g. when a ViewModel uses ApplicationScope, which survives longer than the ViewModel itself
Then you have a running coroutine which maybe holds references to ViewModel members even after the ViewModel has been destroyed
u
yep, but it holds reference to the vm itself, because in order to access a member you need 
this
👍 1
l
We start processes which need to run even after the ViewModel has been destroyed in Repositories. For the ViewModel it's not even a suspending function then.
Copy code
class MyViewModel {
    fun sendFeedback() {
        FeedbackRepository.instance.sendFeedback() // no coroutine scope, not a suspending function
    }
}

class FeedbackRepository(
    private val coroutineScope: CoroutineScope = ApplicationScope.instance
) {
    fun sendFeedback() {
        coroutineScope.launch {
            apiClient.sendFeedback() // suspending function
        }
    }
}
s
But it would be the same (memory leak) when injecting a CoroutineContext, because I believe the running coroutine is tied to the Job of the context?
l
Yes
Generally you wouldn't notice these memory leaks (unless you profile) if the calls anyway end or timeout after some short time and there's not too many of them
👍🏼 1
But they're there
A cleaner approach is to tie the long-running calls to objects that have a longer lifecycle anyway, like e.g. a repository (which in our case lives the whole application lifecycle).
s
Yes, only endlessly running coroutines would introduce a (noticeable) leak.
👍 1
u
It a design bug, coming from not understanding, I’d say a big issue.
👍 1
l
The mix of Android lifecycle, dependency injection and coroutines is really complex to get right.
u
I mean..not that much really, its just this myth of a pattern that needs to be stopped, people just cargocult
s
Sorry, I don't know what "cargocult" is supposed to mean in this context. Anyway, I guess if the 
launch
lambda doesn't reference any properties or methods of the outer class there should be no leaking? I think it's a subtle issue since once you add a reference to an outside property/method you introduce a memory leak. But it's not really obvious that this is going to happen and it might not even be noticeable if the coroutine just runs for a short period of time.
u
Well, thats how capture in lambdas work, nothing to do with coroutines. It will happen 100% of the time, so idk if its non obvious, other than memoryleaks being silent in general
s
Absolutely.
So in my understanding this would not leak 
Example
because the lambda of 
launch
doesn't reference any property or method of 
Example
?
Copy code
class Example(val appScope: CoroutineScope) {

  fun execute() {
    appScope.launch {
      while (true) {
        println("Hello world")
      }
    }
  }
}
Assuming that 
appScope
is a singleton and there's no reference to 
Example
anymore after 
Example(appScope).execute()
. But this would leak:
Copy code
class Example(val appScope: CoroutineScope) {

  val message = "Hello world"

  fun execute() {
    appScope.launch {
      while (true) {
        println(message)
      }
    }
  }
}
u
yes, because property access is 
this.message
technically, so yes it captures this if this was swift and you could tell it to capture only 
message
directly, then it would not be an issue but this is how java/kotlin does it
👍🏼 1
look at bytecode -> java, and there it will be obvious
5 Views
Open in Slack
PreviousNext
Powered by