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

Change default for global vs child coroutines by scoping coroutine builder (structured concurrency) #410

Closed
elizarov opened this Issue Jun 27, 2018 · 63 comments

Comments

Projects
None yet
@elizarov
Copy link
Collaborator

elizarov commented Jun 27, 2018

Background and definitions

Currently coroutine builders like launch { ... } and async { ... } start a global coroutine by default. By global we mean that this coroutine's lifetime is completely standalone just like a lifetime of a daemon thread and outlives the lifetime of the job that had started it. It is terminated only explicitly or on shutdown of the VM, so the invoker had to make extra steps (like invoking join/await/cancel) to ensure that it does live indefinitely.

In order to start a child coroutine a more explicit and lengthly invocation is needed. Something like async(coroutineContext) { ... } and async(coroutineContext) { ... } or async(parent=job) { ... }, etc. Child coroutine is different from a global coroutine in how its lifetime is scoped. The lifetime of child coroutine is strictly subordinate to the lifetime of its parent job (coroutine). A parent job cannot complete until all its children are complete, thus preventing accidental leaks of running children coroutines outside of parent's scope.

Problem

This seems to be a wrong default. Global coroutines are error-prone. They are easy to leak and they do not represent a typical use-case where some kind of parallel decomposition of work is needed. It is easy to miss the requirement of adding an explicit coroutineContext or parent=job parameter to start a child coroutine, introducing subtle and hard to debug problems in the code.

Consider the following code that performs parallel loading of two images and returns a combined result (a typical example of parallel decomposition):

suspend fun loadAndCombineImage(name1: String, name2: String): Image {
    val image1 = async { loadImage(name1) }
    val image2 = async { loadImage(name2) }
    return combineImages(image1.await(), image2.await())
}

This code has a subtle bug in that if loading of the first image fails, then the loading of the second one still proceeds and is not cancelled. Moreover, any error that would occur in the loading of the second image in this case would be lost. Note, that changing async to async(coroutineContext) does not fully solve the problem as it binds async loading of images to the scope of the larger (enclosing) coroutine which is wrong in this case. In this case we want these async operations to be children of loadAndCombineImage operation.

For some additional background reading explaining the problem please see Notes on structured concurrency, or: Go statement considered harmful

Solution

The proposed solution is to deprecate top-level async, launch, and other coroutine builders and redefine them as extension functions on CoroutineScope interface instead. A dedicated top-level GlobalScope instance of CoroutineScope is going to be defined.

Starting a global coroutine would become more explicit and lengthly, like GlobalScope.async { ... } and GlobalScope.launch { ... }, giving an explicit indication to the reader of the code that a global resource was just created and extra care needs to be taken about its potentially unlimited lifetime.

Starting a child coroutine would become less explicit and less verbose. Just using async { ... } or launch { ... } when CoroutineScope is in scope (pun intended) would do it. In particular, it means that the following slide-ware code would not need to use join anymore:

fun main(args: Array<String>) = runBlocking { // this: CoroutineScope 
    val jobs = List(100_000) { 
        launch {
            delay(1000)
            print(".")
        }
    }
    // no need to join here, as all launched coroutines are children of runBlocking automatically
}

For the case of parallel decomposition like loadAndCombineImage we would define a separate builder function to capture and encapsulate the current coroutine scope, so that the following code will work properly in all kind of error condition and will properly cancel the loading of the second image when loading of the first one fails:

suspend fun loadAndCombineImage(name1: String, name2: String): Image = coroutineScope { // this: CoroutineScope 
    val image1 = async { loadImage(name1) }
    val image2 = async { loadImage(name2) }
    combineImages(image1.await(), image2.await())
}

Additional goodies

Another idea behind this design is that it should be easy to turn any entity with life-cycle into an entity that you could start coroutines from. Consider, for example, some kind of an application-specific activity that is launch some coroutines but all of those coroutines should be cancelled when the activity is destroyed (for example). Now it looks like this:

class MyActivity {
    val job = Job() // create a job as a parent for coroutines
    val backgroundContext = ... // somehow inject some context to launch coroutines
    val ctx = backgroundContext + job // actual context to use with coroutines

    fun doSomethingInBackground() = launch(ctx) { ... }
    fun onDestroy() { job.cancel() }    
}

The proposal is to simply this pattern, by allowing an easy implementation of CoroutineScope interface by any business entities like the above activity:

class MyActivity : CoroutineScope {
    val job = Job() // create a job as a parent for coroutines
    val backgroundContext = ... // somehow inject some context to launch coroutines
    override val scopeContext = backgroundContext + job // actual context to use with coroutines

    fun doSomethingInBackground() = launch { ... } // !!! 
    fun onDestroy() { job.cancel() }    
}

Now we don't need to remember to specify the proper context when using launch anywhere in the body of MyActivity class and all launched coroutines will get cancelled when lifecycle of MyActivity terminates.

@fvasco

This comment has been minimized.

Copy link
Contributor

fvasco commented Jun 27, 2018

I would to share some personal consideration about this proposal.

(1) The section "Automated error propagation works" in the linked article is uncovered, so I consider this aspect unchanged.
(2) Instead "Automatic resource cleanup works" should work.

(3) In this proposal and "A surprise benefit: removing go statements enables new features" the Job interface and "coroutine scope" share same aspects, so differences/purposes not look really clear to me. Have these different porposes? Should both merged?
Can we replace the coroutineScope { ... } builder with a job { ... } builder?

(4) Should the sentence

For the case of parallel decomposition like loadAndCombineImage we _would_ define a separate builder function

changed to

For the case of parallel decomposition like loadAndCombineImage we _have to_ define a separate builder function

otherwise we cannot run async code in the same context (might it be the same job?)

(5) The code suspend fun loadAndCombineImage(name1: String, name2: String): Image = coroutineScope { redefines this, I consider the this ridefinition a little error prone for every class's suspending methods.

(6) Should the builder coroutineScope be equivalent to withContext(coroutineContext)?

(7) Is the builder coroutineScope a top level function?
Are GlobalScope.async {...} and coroutineScope { async { ...} } equivalent?

(8) Do the following code

coroutineScope {
    // some code
    coroutineScope {
        /// other code
    }
}

break the rules? (Is "other code" in the global scope?)

@elizarov elizarov changed the title Change default for global vs child coroutines by scoping coroutine builder Change default for global vs child coroutines by scoping coroutine builder (structured concurrency) Jun 27, 2018

@fvasco

This comment has been minimized.

Copy link
Contributor

fvasco commented Jun 28, 2018

I try to reply myself: if the coroutineScope's behaviour and withContext''s behaviour are similar (6) (it looks so) then the answer to (7) is:

No, coroutineScope { async { ...} } is equivalent to withContext (DefaultContext) { ... } but discards the result/error.

the answer to (8) is

No, it create a inner scope and this acts as a barrier for other code

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Jul 6, 2018

@fvasco Let me try to answer:

(1) We also plan for automated error propagation and handling but we don't have a very detailed plan yet. An overall idea is that any scope waits for all its children and if any of those children have unhandled failures (precise definition of this is TBD), then this scope also fails and propagates error upwards.

(2) Our model is that every resource can be represented as a Job and if you create a job that is subordinate to your scope, then there is a guarantee that it gets cancelled (cleaned up) when this scope terminates. That is automated resource cleanup.

(3) We are still struggling how to present the differences between the Job, CoroutineContext, and CoroutineScope and whether some of those concepts should be merged. Current model we have is this:

  • Job is a part of CoroutineContext, but context can contain other items beyond the job.
  • CoroutineScope has a reference to the context and provides coroutine builders as extensions on it.
    So:
    CoroutinesScope - has -> CoroutineContext - has -> Job
    However, the missing part of this picture is that every time you create a new scope, a new Job gets created, too, so, in some sense, CoroutineScope and a Job are the same thing, but if you only have a reference to a Job, you cannot get neither its context nor scope. That's complicated. I don't know at the moment how to make it less confusing.

Using job { ... } as a name of the builder instead of coroutineScope { ... } is one of the options we have on the table.

(4) Good point. Indeed, we "have to"

(5) Redefinition of this is indeed not an ideal solution. We could not find a better solution in the confines of the Kotlin language, so that we can still get the desired syntactic behavior (type-classes would have helped here, but we don't have them now).

(6) Indeed, a coroutineScope builder is conceptually equivalent to withContext(coroutineContext) { .. .}, but the later does not redefine this to CoroutineScope, so you would not be able to directly use async and launch as extensions inside of withContext, but you can do it inside coroutineScope.

(7) coroutineScope is top-level function but the two expressions you gave have different semantics:

  • GlobalScope.async {...} starts a coroutine without parent. You have to explicitly manage its lifecycle, the hardest part of which is that you should not forget to cancel it if you don't need it anymore, even if you crash. You can use this code anywhere (both in suspend and non-suspend functions).
  • coroutineScope { async { ...} } start a coroutine that is subordinate to the delimited coroutineScope block. The scope block cannot terminate until its children async coroutine is working. If something crashes inside this coroutine scope, then async gets cancelled. There is zero risk of forgetting about it (zero risk of leaking it). However, you can use this code only inside suspending function, because coroutineScope is a suspending function.

(8) You can freely nest coroutineScope inside each other without problems. Nothing is going to break.

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Jul 8, 2018

UPDATE: I've added "Additional goodies" to the top message with some additional explanations of the proposed design.

@fvasco

This comment has been minimized.

Copy link
Contributor

fvasco commented Jul 8, 2018

Hi @elizarov,
"Additional goodies" makes me sceptic.

For my point of view MyActivity "is not" a CoroutineScope, moreover implementing CoroutineScope makes the scope public, and this can break OO encapsulation (I can invoke myActivity[Job]?.cancel()).

Seeing the your example, as related consideration, I consider misleading the Job's parent parameter.
You use the context to define the right job, but you can also use parent to override the job (so it is possible define the parent in two ways).
But using launch(ctx, parent = null) does not create a standalone job.
So I propose you to reconsider the parent parameter.

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Jul 10, 2018

@fvasco Ah. Here is confusion of CoroutineScope and CoroutineScope again. Let me clarify.

CoroutineScope is not a CoroutineContext so if you implement CoroutineScope in MyActivity you cannot do myActivity[Job]. But since MyActivity is a CoroutineScope it provide a scope for coroutines. By the word "scope" we mean a "delimited lifetime", so when MyActivity terminates all the coroutines that were launched "in its scope" are cancelled.

Yes. It looks like we should deprecate launch(ctx, parent=...) as part of this proposal.

@fvasco

This comment has been minimized.

Copy link
Contributor

fvasco commented Jul 10, 2018

Here is confusion of CoroutineScope and CoroutineScope again.

I am not alone :)

Sorry, I mean myActivity.scopeContext[Job], becouse scopeContext is public, so -I suspect- everyone can create coroutine in this scope or cancel it.

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Jul 10, 2018

so -I suspect- everyone can create coroutine in this scope or cancel it.

It seems reasonable if the lifecycle of your activity is public to observers, which is usually the case. If the lifecycle is private, then you should hide your coroutine scope, too.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Jul 21, 2018

With Android Architecture Components / AndroidX lifecycles, you can make a Job tied to the lifecycle of an Activity, a Fragment or any LifecycleOwner.
I made a gist for this: https://gist.github.com/LouisCAD/58d3017eedb60ce00721cb32a461980f

This doesn't need to add boilerplate to the Activity.
It should be easily adaptable to the change discussed in this issue by defining coroutine builders (launch & async only, because no runBlocking in an Android Activity) as extensions of Lifecycle and LifecycleOwner.

@pull-vert

This comment has been minimized.

Copy link

pull-vert commented Jul 25, 2018

Hi, I had a weird idea that can target the same purpoise but using a new suspending function for child coroutine builder replacing current one (launch, async...), resulting coroutine would be automatically a child of parent Job. But it needs to rename existing global non suspending coroutine builder to avoid method name conflict (launchGlobal, asyncGlobal...).

Here an example for publish :

Existing function is renamed to publishGlobal (no change except name) :

public fun <T> publishGlobal(
    context: CoroutineContext = DefaultDispatcher,
    parent: Job? = null,
    block: suspend ProducerScope<T>.() -> Unit
): Publisher<T> = Publisher { subscriber ->
    val newContext = newCoroutineContext(context, parent)
    val coroutine = PublisherCoroutine(newContext, subscriber)
    subscriber.onSubscribe(coroutine) // do it first (before starting coroutine), to avoid unnecessary suspensions
    coroutine.start(CoroutineStart.DEFAULT, coroutine, block)
}

And now the new child coroutine builder publish :

suspend public fun <T> publish(
        context: CoroutineContext = DefaultDispatcher,
        parent: Job? = null,
        block: suspend ProducerScope<T>.() -> Unit
): Publisher<T> {
    val parentJob = parent ?: coroutineContext[Job]
    return Publisher { subscriber ->
        val newContext = newCoroutineContext(context, parentJob)
        val coroutine = PublisherCoroutine(newContext, subscriber)
        subscriber.onSubscribe(coroutine) // do it first (before starting coroutine), to avoid unnecessary suspensions
        coroutine.start(CoroutineStart.DEFAULT, coroutine, block)
    }
}

From a non suspending function, the only way to build a coroutine would be to call _____Global coroutine builder, because suspending functions are not available.
This would be clear that it is a global coroutine, it would also be possible to use _____Global coroutine builder when this is explicitly needed inside existing Coroutine.

But this solution does not cover the the case of parallel decomposition like loadAndCombineImage, it would bind async loading of images to the scope of the larger (enclosing) coroutine and not the suspending function only.
Maybe there is something to do to solve this case , perhaps the same solution you showed with a separate builder function to capture and encapsulate the current coroutine Context ?

What do you think about it ?

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Jul 25, 2018

@pull-vert We've considered an option to duplication every builder xxx with xxxGlobal with suspend fun xxx and regular fun xxxGlobal.

However, we had decided that that is going to be too much duplication. Another disadvantage of this xxx and xxxGlobal design is that the scope that is being captured by xxx builders is implicit. Let me explain.

The code can be in the middle of some deep call hierarchy inside suspend fun foo() and when I do launch { ... } from inside of foo I don't want the new coroutine to become a child of some outer Job that had invoked foo() somewhere (and who knows how long that outer coroutine is going to run). I really want the new coroutine to become a child of foo itself, that is, foo shall wait until the coroutines it had launched terminate. Structured concurrency. That is what I want.

In the design that we are currently considering, we don't have any duplication. You can use xxx and Global.xxx. At the same time, we get structured concurrency. You cannot simply invoke launch { ... } from inside of suspend fun foo(). You have to clearly demarcate its scope with coroutineScope { .... }.

@pull-vert

This comment has been minimized.

Copy link

pull-vert commented Jul 25, 2018

Thanks for clarification, I understand that launching coroutine will need a coroutineScope after this evolution : either using CoroutineScope in scope when directly in a parent coroutine, or by clearly demarcate its scope with coroutineScope { .... } in a suspend fun foo()).

A small exemple to see if I understood correctly :-)
exemple 1 : direct children coroutines

fun main(args: Array<String>) = runBlocking { // this: CoroutineScope1
    val jobs = List(100_000) { 
        launch { // extension of CoroutineScope1, child of runBlocking
            delay(1000)
            print(".")
        }
    }
    // no need to join here, as all launched coroutines are children of runBlocking automatically
}

exemple 2 : children coroutines inside suspend fun foo()

fun main(args: Array<String>) = runBlocking { // this: CoroutineScope1
    foo() // just call suspending foo
    // foo() will end only when all launched coroutines are done
}

suspend fun foo() = coroutineScope { // this: CoroutineScope2 : required for calling launch
    val jobs = List(100_000) { 
        launch { // extension of CoroutineScope2, child of suspend foo fun, but not of runBlocking
            delay(1000)
            print(".")
        }
    }
    // no need to join here, as all launched coroutines are children of suspend foo fun automatically
}

@elizarov elizarov self-assigned this Jul 25, 2018

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Jul 25, 2018

@pull-vert Yes. Your examples correctly show proposed enhancement.

@pull-vert

This comment has been minimized.

Copy link

pull-vert commented Jul 29, 2018

@elizarov to be more consistent for coroutine start, as a coroutineScope is supposed to be the...scope of a block of code !
Instead of GlobalScope.launch { ... } maybe it would be better to define it like this ?

fun main(args: Array<String>) {
    globalScope { // this: CoroutineScope
        launch { // extension of CoroutineScope
            ....
        }
    }
}
@matejdro

This comment has been minimized.

Copy link

matejdro commented Jul 29, 2018

GlobalScope.launch looks better to me since it reduces amount of indentation.

@ScottPierce

This comment has been minimized.

Copy link
Contributor

ScottPierce commented Jul 29, 2018

I agree that GlobalScope.launch looks better than coroutineScope, for the same reason, less indentation.

One minor thought is that I'm not in love with the name GlobalScope, as this will become the default entry point to the coroutine library. GlobalScope is descriptive of the context the coroutine is running in, not the entry point that is being used. i.e. if this were a part of the Picasso library, the call would be Picasso.xxx. In RxJava to get the io scheduler, I call Schedulers.io(). Given that the term Scope is so generic, I feel like GlobalScope answers the "where", not the "what", and intuitively I'd expect an API like this to be placed on a "what". It feels a little clunky. I don't really have an alternative to propose, just a thought.

The xxxGlobal also looks good. You mentioned it'd be too much duplication, but that doesn't seem like much duplication at all. Maybe there is something behind the scenes I'm not realizing?.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Jul 29, 2018

@ScottPierce The entry point is likely to be runBlocking for JVM, and Lifecycle/LifecycleOwner on Android, so GlobalScope will probably not be used that much.

@ScottPierce

This comment has been minimized.

Copy link
Contributor

ScottPierce commented Jul 29, 2018

@LouisCAD Not sure I agree with that.

For the JVM, I assume you reference runBlocking because you'd likely use it in your main method to stop the program from exiting? If that's what you mean, for anything more than a single file script, you'd have other files that would be creating and maintaining coroutines, and GlobalScope would need to be used.

For Android, I create coroutines all the time in my client API layer in places outside of a Lifecycle, or LifecycleOwner. I know it's been integrated into the support library as a whole, but I've actually stayed away from the Lifecycle jetpack library because of it's dependency on annotation processing.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Jul 30, 2018

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Aug 6, 2018

@elizarov Roughly each day, I'm thinking about this issue and how much the proposal is better than what we currently have. is there a way to bring experimental implementations with a different name from the final one so we can start trying to use this new coroutine scope thing sooner, and without having to refactor all the code at once?

FYI, what I want to experiment with is bridging Lifecycles from AndroidX/AndroidArchComponents to sort of CoroutineScope and see how I can make the code even more readable and safer with it.

@matejdro

This comment has been minimized.

Copy link

matejdro commented Aug 6, 2018

Just wondering, how does this (and all other interesting improvement discussions here) relate to 1.3 coroutines going stable? Is it good idea to deprecate some functionality (like launch) almost immediately?

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Aug 6, 2018

We plan to have this done before kotlinx.coroutines reaches 1.0.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Sep 28, 2018

@LittleLightCz If you want to change the dispatcher, pass it to async(…) { … }. If you have suspension points into the async blocks though, you can still have parallel execution while a coroutine is suspended.

Kotlin tries to be concise, but explicit. Your proposal is not explicit, so I wouldn't want it, and I wouldn't existing code to change its behavior.

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Sep 28, 2018

@LouisCAD I understand. The thing is, that in the previous version of coroutines I was happily writing async {}, which ran on CommonPool and it mostly was what I wanted. Now it seems I will have to get used to be more explicit :-).

Anyway it would be perfect if the coroutine builder defaults were fine-tuned in a way that the programmer's explicitness was inverse-proportional to the commonness of the problem he/she is trying to slove.

For my happines I would get by just with 1 UI thread and 1 nice IO pool for everything else 😁

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Sep 28, 2018

@LittleLightCz You can still write your own extension named asyncIO { … } or alike to have that default behavior. Before structured concurrency, I had a launchInUi { … } extension to launch on the UI thread on Android and was happy to be able to do this with Kotlin wonderful extensions support.

@GeoffreyMetais

This comment has been minimized.

Copy link

GeoffreyMetais commented Sep 28, 2018

@LittleLightCz you can have the same behavior with coroutineContext = Dispatchers.IO

async with no argument would dispatch in IO dedicated threadpool, and you should have to specify Dispatchers.JavaFx when needed.

Current coroutine behavior is flexible enough to fit your case.

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Sep 28, 2018

@GeoffreyMetais Hmh, maybe you're right :-). Speaking of TornadoFx apps, if all View classes had coroutineContext = Dispatchers.JavaFx and all Controller classes had coroutineContext = Dispatchers.IO, then perhaps I could get the behaviour I need. I will try to fiddle with the code design and I will see - thanks so far! :-)

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Sep 28, 2018

It seems I encountered another caveat:

override val coroutineContext = Dispatchers.JavaFx

launch {
    launch(Dispatchers.IO) { Browser.open(it) }
}

The Browser.open(it) won't get executed at all. However if I change it to GlobalScope.launch(Dispatchers.IO) { Browser.open(it) } then it works. I think that compiler should give me at least some warning, because at that point I had no idea that I was doing something wrong. (Or is this a bug?)

EDIT: It seems it's a bit trickier than I thought - in some cases it works and in other cases it requires GlobalScope prefix. I will try to make a dummy code and reproduce it tomorrow.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Sep 29, 2018

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Sep 29, 2018

It seems it occurs only when I use one unusual design pattern (which however worked wonderfully before 😁). Reproduced here: https://youtrack.jetbrains.net/issue/KT-27281

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Sep 29, 2018

Regarding cancellation, having this pseudo-code:

override val coroutineContext = Dispatchers.JavaFx

val job = launch {
   anotherClass.someSuspendFunc(this)
}

job.cancel()

Do I understand it correctly that if I want to make further nested async {} calls inside that someSuspendFunc cancellable, I need to pass the current CoroutineScope to it and then do something like:

parentScope.run {
    async(IO) { ... }
}

where now the async(IO) { ... } call will be cancelled as well if the original job was cancelled?

@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Sep 29, 2018

@LittleLightCz

which means that although I have 2 async calls in my code, there is nothing really async happening since the used coroutine context is single-threaded, which is super-confusing.

Being able to do many things in the single-thread is the whole point of asynchronous programming (sic!). You only need to care about threads when your code is not asynchronous, that is, when your code is blocking or consuming a lot of CPU.

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Oct 6, 2018

After a week (although actually it took me 1-2days 😁) I migrated one of my smaller project (22 kt files, ~1.2K LOC) to the new structured coroutines and here is my overall feedback:

  • What I like the most about the new coroutines is the easiness of cancellability. It works very nice and it even seems there is no need to do the isActive checks anymore (if I tend to switch contexts more atomically)
  • On the other hand it's a pity that this parent/child relation gets broken when I call a suspend function from a different class. When I want all another nested coroutines inside that suspend function to be cancellable as well, I need to pass the CoroutineScope manually as a parameter. Perhaps in the future this could be done by the compiler automatically?
  • Inside this cancellable suspend function, when I spawn new coroutines on that passed CoroutineScope, I always need to be explicit about on which pool will the new coroutine run since any CoroutineScope can be passed there. Therefore I think it is a good idea that a child coroutine inherits the parent's scope, but not that it also inherits the parent's context/thread pool.
  • I discouraged myself of using the "class that implements CoroutineScope" approach since when I navigated to some suspend function to do some changes and saw a coroutine builder call, at the first look I couldn't tell where this couroutine will run so I always needed to scroll all the way up to check which coroutineContext is defined there. Therefore I tend more to use the GlobalScope. approach.
  • However since writing GlobalScope. is (relatively) a lot of boilerplate, then I decided to go with my own custom set of coroutine builders as @LouisCAD suggested and I've currently settled on these ones which cover almost all of my use cases (+ I am experimenting with the brand new name for the "default withContext" as well 😁):
fun launchUI(block: suspend CoroutineScope.() -> Unit) = GlobalScope.launch(Dispatchers.JavaFx, block = block)

fun launchIO(block: suspend CoroutineScope.() -> Unit) = GlobalScope.launch(Dispatchers.IO, block = block)

fun <T> asyncIO(block: suspend CoroutineScope.() -> T) = GlobalScope.async(Dispatchers.IO, block = block)

fun <T> CoroutineScope.asyncIO(block: suspend CoroutineScope.() -> T) = async(Dispatchers.IO, block = block)

suspend fun <T> offload(block: suspend CoroutineScope.() -> T) = withContext(Dispatchers.IO, block = block)
@elizarov

This comment has been minimized.

Copy link
Collaborator

elizarov commented Oct 6, 2018

@LittleLightCz Can you please, elaborate on this comment:

On the other hand it's a pity that this parent/child relation gets broken when I call a suspend function from a different class. When I want all another nested coroutines inside that suspend function to be cancellable as well, I need to pass the CoroutineScope manually as a parameter. Perhaps in the future this could be done by the compiler automatically?

Can you show a piece of code where you have to do that? Have you tried to use coroutineScope { ... }?

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Oct 6, 2018

@elizarov It seems that I've finally managed to reproduce this behavior on the following code:

import kotlinx.coroutines.*
import kotlinx.coroutines.Dispatchers.IO
import kotlinx.coroutines.Dispatchers.Default

class Foo() {
    suspend fun processIds() {
        (1..1000).map { id ->
            GlobalScope.async(IO) {
                Thread.sleep(200)
                println("Processed id $id")
            }
        }.awaitAll()
    }
}

fun main() {
    val job = GlobalScope.launch(Default) {
        Foo().processIds()
    }

    Thread.sleep(1000)
    job.cancel()
}

When the job.cancel() is called, it seems that async(IO) coroutines continue to run. However if I do:

class Foo() {
    suspend fun processIds(parentScope: CoroutineScope) {
        (1..1000).map { id ->
            parentScope.async(IO) {
                Thread.sleep(200)
                println("Processed id $id")
            }
        }.awaitAll()
    }
}

fun main() {
    val job = GlobalScope.launch(Default) {
        Foo().processIds(this)
    }

    Thread.sleep(1000)
    job.cancel()
}

Then it stops at around ID of 320. BTW I haven't used coroutineScope { ... } yet, so unfortunately I don't know what do you mean - can you please give me a hint how it could be used here?

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Oct 6, 2018

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Oct 7, 2018

@LouisCAD Thanks, found it ☺️ (https://medium.com/@elizarov/structured-concurrency-722d765aa952). So what I did was just simple:

    suspend fun processIds() = coroutineScope {
        (1..1000).map { id ->
            async(IO) {
                Thread.sleep(200)
                println("Processed id $id")
            }
        }.awaitAll()
    }

and now it really works indeed! That's nice ☺️. I recall that I read Roman's article back then when it was released but this detail sort of vanished from my mind - now I will hopefully remember 😉.

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Oct 7, 2018

Btw the above implies that one of my own custom functions is not needed anymore 🤔

fun <T> asyncIO(block: suspend CoroutineScope.() -> T) = GlobalScope.async(Dispatchers.IO, block = block)

Refactored the rest of my code and it looks cleaner now - the coroutineScope { ... } was a smart design move ☺️. I always say that JetBrains as a company is named like that for a reason 😉 😁.

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Oct 13, 2018

The evolution continues. I got rid of my previous GlobalScope extensions, where now I incline more to define my own scopes as vals + use the invoke operator overload (one of the "crazy" ideas that Roman already mentioned in different thread - I think) for launch by default. Currently I am at:

val UI = object : CoroutineScope {
    override val coroutineContext = Dispatchers.JavaFx
}

val IO = object : CoroutineScope {
    override val coroutineContext = Dispatchers.IO
}

operator fun CoroutineScope.invoke(block: suspend CoroutineScope.() -> Unit) = launch(block = block)

fun <T> CoroutineScope.asyncIO(block: suspend CoroutineScope.() -> T) = async(Dispatchers.IO, block = block)

suspend fun <T> offload(block: suspend CoroutineScope.() -> T) = withContext(Dispatchers.IO, block = block)
@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Oct 17, 2018

Hi @elizarov, in the last few days I was thinking about your comment you gave me on your article (https://medium.com/@elizarov/futures-cancellation-and-coroutines-b5ce9c3ede3a) and as a conclusion I found a way how to progress into more idiomatic coroutines code and got rid of my fun <T> CoroutineScope.asyncIO() extension function mentioned above using the combo of: withContext(IO) { async { ... }}. If I understood you right, you were talking about the async { withContext() { ... }} way, but I hope that even the reversed approach is ok (idiomatic) too ☺️. The main concept that I bear in my mind from now on is that the suspend function is responsible for non-blocking and it seems it works fine so far.

@fvasco

This comment has been minimized.

Copy link
Contributor

fvasco commented Oct 17, 2018

@LittleLightCz consider the differences between withContext(IO) { async { ... }}, async { withContext(IO) { ... }} and async(IO) { ... }

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Oct 18, 2018

@fvasco Well, since for withContext(IO) {} I have this custom shortcut: offload {}, then taken the scenario when I want to use async inside a suspend function, my real code looks like this:

Before:

suspend fun doSomething() = coroutineContext {
    .
    .
    .
    asyncIO { ... }
}

After:

suspend fun doSomething() = offload {
    .
    .
    .
    async { ... }
}

offload is shorter than coroutineContext and async is shorter than asyncIO, so I think this is a win-win ☺️ 👏

@fvasco

This comment has been minimized.

Copy link
Contributor

fvasco commented Oct 18, 2018

@LittleLightCz the two coroutines are different different, these use different dispatchers.
Moreover put the async at the end of code (look at example) does not look really useful to me.
I am confused.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Oct 18, 2018

@LittleLightCz If you told me you understood the purpose of the async function, I would have a hard time believing you. I use it only for parallel (non sequential) coroutines execution.

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Oct 19, 2018

@fvasco @LouisCAD Don't worry gentlemen, I believe I use coroutines in the same way as you do ☺️, but that wasn't my point. Let's just disregard there is no .await() etc. and if I sum it up, then all I wanted to say is that: "the second approach is more concise/better than the first one". In other words I find it slightly better practice to switch the context to IO first and then use async {}, than to use the coroutineContext and then call e.g. async(IO) {}, that is all ☺️.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Oct 19, 2018

@LittleLightCz

This comment has been minimized.

Copy link

LittleLightCz commented Oct 19, 2018

@LouisCAD If you mean to let the whole class implement the CoroutineScope, then I've already explained why I don't want to use it in one of my previous comments here: #410 (comment)

TL;DR: I don't want to scroll all the way up to see which scope it is each time I open any method of that class ☺️, that is just horrifying. Plus I am not sure if newly spawned coroutines inside suspending functions of such class would be automatically cancellable - I haven't tried it, so I can't confirm how would the coroutines behave. Anyway when coroutineContext {} or the withContext(IO} { //async/launch etc... } combo is used, they cancel like a charm ☺️ ❤️.

@LouisCAD

This comment has been minimized.

Copy link
Contributor

LouisCAD commented Oct 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment