https://kotlinlang.org logo
Join Slack
Powered by
I vaguely remember reading that it doesn’t make se...
# coroutines
m
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
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
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
@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
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
The class implements an interface that has non-suspending functions - fire-and-forget
a
is it a listener or callback of some sort?
m
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
so a long-lived reference to it will definitely escape to some other external bit of code then
m
I’m not sure what you mean. When the viewmodel dies, the channel is closed and the audiotrack is torn down
a
by what mechanism? It looks like it has to be externally managed from the example
m
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
I see, but you need a suspending call to get some dependency for the 
MyClass
m
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
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
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
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
Ok, and I think in this case, this is not what I want
a
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
Yes, right
a
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
Or can MyClass just listen for when the coroutinescope is cancelled?
Would still need a scope to create the actor
a
or at least a dispatcher, yeah.
m
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
right but you probably also want a well-defined place to catch those and deal with them
m
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
oh, yes that is. At that point if 
createMyClass
doesn't need to suspend itself, it shouldn't be marked 
suspend
m
But it has to call createMyArg()
a
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
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
ideally not, if possible.
m
Is there anyway to define createMyClass() as non-suspending, accepting a coroutinescope and returning MyClass?
a
yes, but it would need to drop the internal call to 
createMyArg
m
Maybe returning 
Deferred<MyClass>
is a better way
a
I think that would complicate things further
m
Maybe better to make MyClass lazily instantiate MyArg
a
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
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
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
Oh I see
a
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
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
does 
myArg
even have to be a property in that version or is it only used within the actor?
m
Perhaps not - I only keep a reference so it can be torn down when MyClass is torn down
a
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
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
at minimum it looks like it doesn't even need to be a property anymore, just a constructor parameter 🙂
m
I haven’t included the play(), stop() functions for sending asynchronously 🙂
a
add a buffer size to the actor creation and you can 
offer
instead to do that though
m
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
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
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
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
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
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
Couldn’t that be achieved by something like 
fun playAsync(): Job
?
a
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
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
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
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
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
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
no problem that you won't already have with piled up launches
m
maybe even also with WRITE_NON_BLOCKING since I think that blocks when the buffer is full
2 Views
Open in Slack
PreviousNext
Powered by