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

Extensions on CoroutineScope to improve its mental model #828

Closed
qwwdfsad opened this issue Nov 13, 2018 · 8 comments
Closed

Extensions on CoroutineScope to improve its mental model #828

qwwdfsad opened this issue Nov 13, 2018 · 8 comments
Assignees

Comments

@qwwdfsad
Copy link
Member

qwwdfsad commented Nov 13, 2018

We see a lot of confusion around the fact that the library has two very similar entities exposed in various ways (e.g. all builders are extensions on CoroutineScope, but have CoroutineContext argument)

While they, in fact, have different semantics, the question is how to educate the users in a most natural way without forcing them to read a lot of documentation in the guide.

As a first step, we could add following extensions to CorouineScope (and mark them with DslMarker to help autocompletion):

val job: Job
val jobOrNull: Job?

val dispatcher: CoroutineDispatcher
val dispatcherOrNull: CoroutineDispatcher?
cancel()
@elizarov
Copy link
Contributor

I don't see see compelling use-cases for the first 4 extensions (job, jobOrNull, dispatcher, dispatcherOrNull), but there is indeed a compelling use-case for cancel() in light of #829, albeit there is a naming problem. Such a short name in the scope would invariably conflict with other methods called cancel() in scenarios like this:

launch { // note: this: CoroutineScope
    // ... other code
    cancel() // what are we cancelling here? 
}

So I'd suggest to call it cancelCoroutineScope(). See #829 for details.

@fvasco
Copy link
Contributor

fvasco commented Nov 15, 2018

It is possible to avoid cancel/cancelCoroutineScope' at all and propose an explicit job.cancel()`.

launch { // note: this: CoroutineScope
    // ... other code
    job.cancel()
}

Personally I consider this path cleaner (and shorter).

@elizarov
Copy link
Contributor

@fvasco job.cancel() looks cleaner indeed, but it poses two problems:

  • job is a very short name, so if we introduce CoroutineScope.job extension at this point we would break a lot of existing code that references its own job, because with this extension job will start resolving to a different thing when used inside of launch { ... }. It will be very hard for people to track down and fix.

  • job.cancel() idiom would require people who are new to coroutines to understand the concept of Job in the first place, while we are actually trying to reduce the number of concepts that novice developers need to understand to get started (see Provide MainScope factory #829 for more details).

@fvasco
Copy link
Contributor

fvasco commented Nov 16, 2018

@elizarov, I agree with you on first point, the above functions can become an extensions on CoroutineContext (but coroutineContext.job is not so much different by coroutineContext[Job]!!).

Regarding the second point, I not agree yet (#410 (comment)).
If an Activity is a CoroutineScope then cancel() is the obvious name (similar to close for a stream or release for a buffer).
Understanding a framework is only a first part of the job.

@LouisCAD
Copy link
Contributor

LouisCAD commented Nov 21, 2018

In case of GlobalScope, or other CoroutineScope without a backing Jobin its coroutineContext, the discussed val job: Job property would always throw, which is far from ideal.

val jobOrNull: Job? doesn't have this problem (unless you are very assertive!!), and is unlikely to break existing code.

Maybe a migration could inspect code, searching for potential conflicts leading to unintended behavior still?

@dave08
Copy link

dave08 commented Nov 22, 2018

Or maybe there could be a "light" implementation of job that just cancels.. The whole idea is to make things simpler, if you have to check for nulls (or know when to do that for beginners), you lost part of the advantage.

@elizarov
Copy link
Contributor

We've decided to start with an experimental CoroutineScope.cancel() extension and see how it works. We'll make it inline so that we can renamed in the future if problems with such a short name are found. Making it experimental would help notice accidental naming conflicts (if any).

@qwwdfsad
Copy link
Member Author

We do have cancel, job and context.[CoroutineDispatcher] now, closing as obsolete/fixed

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

5 participants