https://kotlinlang.org logo
#coroutines
Title
# coroutines
m

Mark

12/31/2019, 2:58 AM
I vaguely remember reading that it doesn’t make sense to define a suspending function that accepts a
CoroutineScope
as an argument. In my case, I want to create a factory method to instantiate `MyClass`:
Copy code
suspend fun createMyClass(scope: CoroutineScope): MyClass {
	return MyClass(scope, createMyArg())
}

suspend fun createMyArg(): MyArg {...}

class MyClass(
    private val scope: CoroutineScope,
    private val myArg: MyArg
)
and I would use it from my
AndroidViewModel
like:
Copy code
viewModelScope.launch {
    createMyClass(this).doSomething()
    // do some more stuff
}
What is the proper way to write
createMyClass()
? For example, how about this?
Copy code
suspend fun createMyClass(): MyClass {
	return MyClass(CoroutineScope(coroutineContext), createMyArg())
}
m

molikuner

12/31/2019, 3:29 AM
I would argue, that this really depends on the use case. The first example could be useful if you want to create the class in a background thread but execute everything inside the class in an other thread. For example the UI thread. The second version would pass the current execution context into the class instance. Maybe I would rename the argument in the first version from
scope
to
instanceScope
to clarify the intended use. Otherwise one could misunderstand that this was the scope that the function would be executed in. This indeed wouldn’t make any sense because of the suspend modifier. Apart from all that I would make the scope a receiver to somehow follow the convention. A function, that launches it’s own coroutines (this time the class probably launches them) and returns earlier, than those coroutines end, is an extension function on a `CoroutineScope`:
Copy code
suspend fun CoroutineScope.createMyClass(): MyClass {
	return MyClass(this, createMyArg())
}
1
a

Adam Powell

12/31/2019, 3:32 AM
constructing an object giving it a
CoroutineScope
implies all sorts of things about what that object might do to it and what valid states of that object are
take the example you have there; what if that code were written:
Copy code
viewModelScope.launch {
  val myObj = createMyClass(this)
  myObj.doSomething()
  someOtherObject.obj = myObj
}
in a situation like this, the
CoroutineScope
given to it can be completed or cancelled even though a reference to the object escaped to somewhere else in your system. How does a
MyClass
instance behave when this happens?
m

Mark

12/31/2019, 3:35 AM
@Adam Powell for some context,
MyClass
is wrapping a
Channel
and declares functions that launch coroutines (using passed-in scope) to send to that channel.
a

Adam Powell

12/31/2019, 3:35 AM
so is the
MyClass
only taking a scope for the sake of injecting context elements from elsewhere?
Can the functions on
MyClass
themselves be declared
suspend
instead of giving a
CoroutineScope
to
MyClass
?
m

Mark

12/31/2019, 3:37 AM
The class implements an interface that has non-suspending functions - fire-and-forget
a

Adam Powell

12/31/2019, 3:39 AM
is it a listener or callback of some sort?
m

Mark

12/31/2019, 3:41 AM
The interface is for audio playback and so has methods like play(), stop() etc. The class implements these by sending messages to the wrapped channel.
The class uses an actor to read from this channel and write to an
AudioTrack
a

Adam Powell

12/31/2019, 3:43 AM
so a long-lived reference to it will definitely escape to some other external bit of code then
m

Mark

12/31/2019, 3:45 AM
I’m not sure what you mean. When the viewmodel dies, the channel is closed and the audiotrack is torn down
a

Adam Powell

12/31/2019, 3:46 AM
by what mechanism? It looks like it has to be externally managed from the example
m

Mark

12/31/2019, 3:48 AM
Ah yes, I should’ve also mentioned, a reference to MyClass is held by the viewmodel (var property where we check for null for whether to instantiate). Then torn down in ViewModel.onCleared()
a

Adam Powell

12/31/2019, 3:49 AM
I see, but you need a suspending call to get some dependency for the
MyClass
m

Mark

12/31/2019, 3:52 AM
Right, so it’s more like:
Copy code
var myClass: MyClass? = null

fun onSomething() {
    viewModelScope.launch {
		if (myClass == null)
		    myClass = createMyClass(this)
		}
		myClass.doSomething()
	}
}
although this code doesn’t fill me with confidence!
a

Adam Powell

12/31/2019, 3:53 AM
given the circumstances and the tight coupled scope you have here, plus the android platform dependency on audio interface types anyway, I probably wouldn't worry too much about this bootstrapping being kind of janky. You're not going to get a huge value out of testability or something by making this super clean
that said 🙂
I really like the pattern of turning this sort of thing inside out into a scoping function of its own that represents the object lifetime for something like this
the reason the above is janky is because it's hard to tell who owns what scope where
who's responsible for closing/cancelling it, who can close/cancel it
how easy/suggested is it for references to escape
m

Mark

12/31/2019, 3:56 AM
Ok thanks Adam. So are you saying in general it’s not a good idea to pass in a coroutineScope, rather better to create the scope like in the last snippet of my OP?
a

Adam Powell

12/31/2019, 3:56 AM
if you have a
suspend fun
you expect that it does all of its work and either returns or throws; that it doesn't leak launched coroutines into other scopes as a side effect
👍 1
the
createMyClass()
from the last snippet in the OP violates that expectation
it leaks the ability to launch into its caller's parent scope after it returns
at least when you pass the scope explicitly, you kind of know what you're signing up for
the pattern I was referring to above though is more like what
coroutineScope {}
or
withContext(...) {
do - they suspend until all work from the body lambda block is done
m

Mark

12/31/2019, 4:02 AM
Ok, and I think in this case, this is not what I want
a

Adam Powell

12/31/2019, 4:02 AM
perhaps not in your case
it seems like you have a type that does a specific job, you want to bind it to an external
CoroutineScope
, and it's expected that the external owner of that scope can and will cancel it later
m

Mark

12/31/2019, 4:03 AM
Yes, right
a

Adam Powell

12/31/2019, 4:04 AM
I assume
offer
to send messages to that channel isn't really feasible here to cut this dependency? An unlimited-buffer channel vs. launching a bunch of sends isn't going to behave that much differently in practice here but it might help clean this up a bit
m

Mark

12/31/2019, 4:04 AM
Or can MyClass just listen for when the coroutinescope is cancelled?
Would still need a scope to create the actor
a

Adam Powell

12/31/2019, 4:05 AM
or at least a dispatcher, yeah.
m

Mark

12/31/2019, 4:07 AM
Also I want to make sure that if there is an exception (e.g. writing to the AudioTrack), that it doesn’t cancel the viewModelScope
a

Adam Powell

12/31/2019, 4:09 AM
right but you probably also want a well-defined place to catch those and deal with them
m

Mark

12/31/2019, 4:10 AM
Ok, but is it good practice to create a child job for this?
BTW, the “the
createMyClass()
from the last snippet” doesn’t actually launch other coroutines, but rather returns a class that does that when invoked to do so. Isn’t that a key difference?
a

Adam Powell

12/31/2019, 4:14 AM
oh, yes that is. At that point if
createMyClass
doesn't need to suspend itself, it shouldn't be marked
suspend
m

Mark

12/31/2019, 4:15 AM
But it has to call createMyArg()
a

Adam Powell

12/31/2019, 4:17 AM
oh I misread, you said it just doesn't launch
it does in a way though, it delegates the ability to launch into the parent scope after
createMyClass
returns
which
MyClass
then very much does 🙂
m

Mark

12/31/2019, 4:20 AM
Oh yes of course you’re right, not least because of the actor{} initialised by
MyClass
!
okay, so it definitely should not be a suspend function
a

Adam Powell

12/31/2019, 4:21 AM
ideally not, if possible.
m

Mark

12/31/2019, 4:21 AM
Is there anyway to define createMyClass() as non-suspending, accepting a coroutinescope and returning MyClass?
a

Adam Powell

12/31/2019, 4:22 AM
yes, but it would need to drop the internal call to
createMyArg
m

Mark

12/31/2019, 4:22 AM
Maybe returning
Deferred<MyClass>
is a better way
a

Adam Powell

12/31/2019, 4:23 AM
I think that would complicate things further
m

Mark

12/31/2019, 4:26 AM
Maybe better to make MyClass lazily instantiate MyArg
a

Adam Powell

12/31/2019, 4:28 AM
one way to simplify and clean things up a bit is to try to get the setup and teardown to be handled more symmetrically, and let structured concurrency do the work. Consider:
Copy code
viewModelScope.launch {
  val myObj = MyClass(createMyArg())
  launch { myObj.run() }
  myObj.doStuff()
}
where
run
will receive the inbound messages from the channel and handle them; essentially being the actor
either push more of the lifecycle management into
MyClass
or lift it out; it's the straddling that makes things awkward
👍 1
and since you're controlling most of this through the
viewModelScope
, pushing more of it into
MyClass
I think is going to be more difficult than the alternative
An advantage to something like lifting the actor processing out that way is that you can then do things like this:
Copy code
launch {
  try {
    myObj.run()
  } catch (e: MyException) {
    // recover, log, retry...
  }
}
👍 1
and you sidestep any of the question of how to make sure a known error doesn't bring down your
viewModelScope
while still getting the error handling benefits of coroutines
m

Mark

12/31/2019, 4:35 AM
Ok, but that’s just sending to the channel. Actually creating the channel/actor requires a scope which either needs to be passed into the constructor or via that
run()
method (in the case where you don’t want MyClass holding a scope reference).
a

Adam Powell

12/31/2019, 4:37 AM
the key is that
.run()
doesn't have to return until the actor completes. Or "actor" if you eliminate the extra launch and just have
run
loop on channel receive and be the actor itself rather than using the actual
actor {}
api that launches
👍 1
m

Mark

12/31/2019, 4:38 AM
Oh I see
a

Adam Powell

12/31/2019, 4:42 AM
use
channel.consumeEach {}
or
.consumeAsFlow()...collect {}
in there and you get the channel closing when the whole thing tears down like you described too, without having to register or do it elsewhere
m

Mark

12/31/2019, 4:52 AM
Ok, I’ll look into that approach shortly, but first I wanted to share the solution where
createMyArg()
is called within
MyClass
because it tidies things up significantly.
Copy code
suspend fun createMyArg(): MyArg

class MyClass(private val scope: CoroutineScope) {
    private lateinit var myArg: MyArg
    
    private val sendChannel: SendChannel<Msg> = scope.actor {
    	myArg = createMyArg()
	    for (msg in channel) { // iterate over incoming messages
	    ...
	    }
    }
}

var myClass: MyClass? = null

fun onSomething() {
	if (myClass == null)
	    myClass = createMyClass(viewModelScope)
	}
	myClass.doSomething()
}

fun createMyClass(scope: CoroutineScope): MyClass {
	return MyClass(scope)
}
a

Adam Powell

12/31/2019, 4:53 AM
does
myArg
even have to be a property in that version or is it only used within the actor?
m

Mark

12/31/2019, 4:55 AM
Perhaps not - I only keep a reference so it can be torn down when MyClass is torn down
a

Adam Powell

12/31/2019, 4:55 AM
or even
scope
for that matter if it's only used to launch the actor on construction and you buffer it so that you can
offer
instead of `launch`/`send`?
can you tear it down in a try/finally or
.use {}
around the body of the actor loop instead?
m

Mark

12/31/2019, 5:01 AM
Copy code
suspend fun createMyArg(): MyArg

class MyClass(private val scope: CoroutineScope) {
    private val sendChannel: SendChannel<Msg> = scope.actor {
    	val myArg = createMyArg()
    	try {
	        for (msg in channel) { // iterate over incoming messages
	        ...
    	    }
    	} finally {
	    	myArg.tearDown()
    	}
    }
}

var myClass by lazy { MyClass(viewModelScope) }

fun onSomething() {
	myClass.doSomething()
}
That just leaves the scope - whether to externalise it. I’ll play with both approaches
a

Adam Powell

12/31/2019, 5:02 AM
at minimum it looks like it doesn't even need to be a property anymore, just a constructor parameter 🙂
m

Mark

12/31/2019, 5:02 AM
I haven’t included the play(), stop() functions for sending asynchronously 🙂
a

Adam Powell

12/31/2019, 5:04 AM
add a buffer size to the actor creation and you can
offer
instead to do that though
m

Mark

12/31/2019, 5:18 AM
I currently use a buffer size of 3, but would rather not increase the size much because potentially there could be a lot of messages sent to that channel and I don’t want to process those messages unnecessarily
a

Adam Powell

12/31/2019, 5:22 AM
vs. queuing up the same number of fire-and-forget launched coroutines with the same effect?
in that scenario you're just using the coroutine dispatcher as an unlimited buffer
m

Mark

12/31/2019, 6:16 AM
In general, I feel that the scope used to build the actor should be the same as the scope used to send messages to the channel. When would it make sense to use a different scope? Wouldn’t there be a potential for sending to an already closed channel, for example?
a

Adam Powell

12/31/2019, 6:19 AM
any time you launch you've created a new scope; the only reason to ever use a channel in the first place is to communicate with something outside of your own scope
m

Mark

12/31/2019, 6:23 AM
But such a launch scope would be a descendant of
viewModelScope
and so if
viewModelScope
was cancelled, then everything would be more consistent. With an API like
CoroutineScope.playAsync()
- there would potentially be calls like
GlobalScope.playAsync()
a

Adam Powell

12/31/2019, 6:30 AM
that doesn't add any consistency, it only cuts off the ability for the sender-caller to receive any sort of feedback after it shouted into the void 🙂
m

Mark

12/31/2019, 6:31 AM
Couldn’t that be achieved by something like
fun playAsync(): Job
?
a

Adam Powell

12/31/2019, 6:32 AM
always favor
suspend fun
over returning
Deferred
or
Job
and performing a launch under the hood, it's so much easier to handle everything
m

Mark

12/31/2019, 6:33 AM
I agree in general, but in the case where a call really is fire-and-forget and the caller really doesn’t need any feedback?
a

Adam Powell

12/31/2019, 6:33 AM
much easier to reason about as well
then the caller can make the decision to
GlobalScope.launch {}
and treat it that way if they really think that's the case they have
or it can be modeled as a non-suspending
offer
to a channel 🙂
I'm not sure where the resistance to
offer
is coming from and the associated desire to involve more coroutine launches as an alternative, especially when there's already a channel in play that guarantees serialization, etc.
use
suspend
and call
send
if you want to have a small/no buffer and let the caller decide to cancel because it doesn't care anymore, or use
offer
if you want it to be fire and forget and you don't want to (or can't) suspend for a send and set the buffer policy accordingly
m

Mark

12/31/2019, 6:43 AM
The problem I have with offer is that then I have to code for the situation where the buffer is full (returns false). And I don’t want to use a large buffer.
a

Adam Powell

12/31/2019, 6:45 AM
an unlimited buffer will just use a linked list, which is relatively what you get when you're posting the same number of coroutine launches to a dispatcher
you're not saving anything efficiency-wise, the launches are probably less efficient
m

Mark

12/31/2019, 6:46 AM
It’s the reading side I’m concerned about. Wouldn’t there be a problem with piling on loads of data to the
AudioTrack
?
a

Adam Powell

12/31/2019, 6:47 AM
no problem that you won't already have with piled up launches
m

Mark

12/31/2019, 6:48 AM
maybe even also with WRITE_NON_BLOCKING since I think that blocks when the buffer is full
2 Views