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

Consider removing the context parameter in coroutine builders #10

Open
RBusarow opened this issue Jan 2, 2020 · 1 comment
Open

Consider removing the context parameter in coroutine builders #10

RBusarow opened this issue Jan 2, 2020 · 1 comment
Labels
enhancement New feature or request

Comments

@RBusarow
Copy link
Owner

RBusarow commented Jan 2, 2020

As of 1.0.0-beta01, builders such as launchDefault(...) take a first parameter of context: CoroutineContext = EmptyCoroutineContext. The full code:

public fun CoroutineScope.launchDefault(
  context: CoroutineContext = EmptyCoroutineContext,
  start: CoroutineStart = CoroutineStart.DEFAULT,
  block: suspend CoroutineScope.() -> Unit
): Job = launch(coroutineContext.dispatcherProvider.default + context, start, block)

This is to preserve the same signature of the original functions (async(...) is the same) as in the kotlinx-coroutines library. Except even Roman says that was a mistake and it only exists today for backwards compatibility.

These new functions are a perfect excuse to drop the parameter, except that there are still other uses for that context other than incorrectly specifying the Job, or overwriting the dispatcher. What about passing in a custom exception handler or a CoroutineName for debugging? Or what about other custom CoroutineContext.Elements?

One option would be to add some require(...)'s to the functions, which throw an exception if the passed CoroutineContext contains a Job or a ContinuationInterceptor. This is done in a few places in the Flow api. But then we're introducing a runtime exception instead of a checked one.

Another option might be to ignore any Job or ContinuationInterceptor properties in the passed context, adding a warning to the comments. This has its own issues, since technically we add some complexity to the fold, and if the user doesn't read the comment, they may miss that their Job or ContinuationInterceptor is being ignored when that's what they really wanted.

I'm leaning toward the require(...) but still undecided.

@RBusarow RBusarow added the enhancement New feature or request label Jan 2, 2020
@damianw
Copy link

damianw commented Jun 26, 2020

Except even Roman says that was a mistake and it only exists today for backwards compatibility.

Do you have a reference for this, by chance? 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants