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

Children factories as extensions on CoroutineScope #688

Closed
qwwdfsad opened this issue Oct 9, 2018 · 7 comments
Closed

Children factories as extensions on CoroutineScope #688

qwwdfsad opened this issue Oct 9, 2018 · 7 comments

Comments

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Oct 9, 2018

The current approach to structured concurrency forces to write a lot of boilerplate while building scoped hierarchies such as val job = Job(corotineContext[Job]).

We should provide a more friendly way to create nested entities:

fun CoroutineScope.childJob(): Job
fun CoroutineScope.childSupervisorJob(): SupervisorJob
fun CoroutineScope.childCompletableJob(): CompletableJob
fun CoroutineScope.childScope(): CoroutineScope

Also, we could (should?) assert that scope has a Job and maybe provide a way to extract dispatcher from the scope.

@fvasco
Copy link
Contributor

fvasco commented Oct 9, 2018

I want to propose again a previous consideration about the scope (3 in #410 (comment))

Is it possible to reconsider CoroutineScope in favour to Job?
Job interface can extend CoroutineScope, so launch, produce and the above functions can become as extension of Job, and we do not have to assert nothing about CoroutineScope.

Moreover the GlobalScope can be easily replaced by a wild Job (Job().launch { ... }).

Finally CoroutineScope does not add more than the core function coroutineContext, probably not so usefull.

@qwwdfsad
Copy link
Collaborator Author

qwwdfsad commented Oct 9, 2018

Is it possible to reconsider CoroutineScope in favour to Job?

It is not. There is a lot of issues, e.g. what if you want to use uiScope which represents your Activity's job and dispatcher? Not talking about the fact that it's impossible to implement Job outside of kotlinx.coroutines and mixing responsibilities.

Moreover the GlobalScope can be easily replaced by a wild Job (Job().launch { ... }).

Not sure why it is useful.

Finally CoroutineScope does not add more than the core function coroutineContext, probably not so useful.

True, there is some confusion between CoroutineScope and CoroutineContext.

Scope is just a box around context with its own type because CoroutineContext contains a lot of irrelevant stuff. E.g. to implement context, you should provide operator get, fold and minusKey methods (what for?). Being a stdlib class, it has a lot of different extensions in the wild (and probably with different meanings specific to each particular project), so having library extensions on CoroutineContext is just tooling unfriendly. It is unobvious when you can both call produce and fold on the same class. Same about providing CoroutineContext as builders receiver, it's really unobvious when you provide cc as receiver and can write EmptyCoroutineContext.launch.

Moreover, CoroutineContext has a completely different mental model. It's just a persistent map, while CoroutineScope has a well-defined meaning in kotlinx.coroutines.

we do not have to assert nothing about CoroutineScope.

This is a grey area actually, I'm not sure we should.

Anyway, this issue is a wrong place to discuss it, I expect it to be an umbrella for small syntactic improvements rather than the discussion about scopes design, let's keep it clear.

@fvasco
Copy link
Contributor

fvasco commented Oct 9, 2018

@qwwdfsad thank for your patience.

Jobs and Dispatchers are both really important for this library.
For Dispatcher was defined a good default, Job gets only a workaroud (5c44afb).

Some library method requires a Job in the context to work property, like CoroutineContext.cancel()

In my opinion CoroutineScope should own a Job, always.
Without a job there is no way to cancel a CoroutineScope.

It is possible to reconsider coroutineJob property.

@elizarov
Copy link
Contributor

@qwwdfsad We should think twice before adding those (take a look and actual use-cases again), because those extensions are dangerous, see:

launch {
    childJob() // ops... lost a reference to it...
}

Now my parent job never completes and how am I supposed to debug that in the large piece of code?

@elizarov
Copy link
Contributor

@fvasco CoroutineScope is an interface with an intention that it always has a Job, but we have limited capacity to actually guarantee that it had a job when you implement it yourself. However, all the CoroutineScope implementations that we provide in kotlinx.coroutines always have a Job.

@fvasco
Copy link
Contributor

fvasco commented Oct 10, 2018

Nice @elizarov

we have limited capacity to actually guarantee that it had a job when you implement it yourself

There is no rules about it in the official documentation, you should mention this intention.

Moreover it will possible to define:

val CoroutineScope.coroutineJob : Job get() = checkNotNull(coroutineContext[Job]) { "No Job founds in this coroutine scope"}

So we can consider to deprecate:

val CoroutineContext.isActive
fun CoroutineContext.cancel*

Because it is not possible to assume any Job in a CoroutineContext, furthermore this functions are already provided by coroutineJob.

@qwwdfsad
Copy link
Collaborator Author

Decided not to implement as too dangerous and error-prone

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

No branches or pull requests

3 participants