Skip to content

How to express intersection of two CoroutineScopes? #814

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

Open
BoxResin opened this issue Nov 9, 2018 · 7 comments
Open

How to express intersection of two CoroutineScopes? #814

BoxResin opened this issue Nov 9, 2018 · 7 comments

Comments

@BoxResin
Copy link

BoxResin commented Nov 9, 2018

I want to use CoroutineScope that is only working when two Jobs are active.

Example

class Something : CoroutineScope {
    // onBind ~ onUnbind
	private var scopeJobBind = Job().apply { cancel() }

	// onResume ~ onPause
	private var scopeJobLifecycle = Job().apply { cancel() }

	// Intention: (onBind ~ onUnbind) ∩ (onResume ~ onPause)
	override val coroutineContext get() = Dispatchers.Main + scopeJobBind + scopeJobLifecycle // The problem: scopeJobLifecycle replaces scopeJobBind

	fun onBind() {
		scopeJobBind = Job()
	}

	fun onUnbind() {
		scopeJobBind.cancel()
	}

	fun onResume() {
		scopeJobLifecycle = Job()
	}

	fun onPause() {
		scopeJobLifecycle.cancel()
	}

	fun foo() {
		// It should be launched only when both scopeJobBind and scopeJobLifecycle are active.
		launch {
			...
		}
	}
}
@BoxResin BoxResin changed the title How to express an intersection of two CoroutineScopes? How to express intersection of two CoroutineScopes? Nov 9, 2018
@elizarov
Copy link
Contributor

elizarov commented Nov 9, 2018

There is no ready-to-use public API for that at the moment, as it raises lots of non-trivial questions with respect to the exception handling. Can you please, clarify your use-case. Why would exactly needed it?

@BoxResin
Copy link
Author

@elizarov
I have two situations where I need to use different CoroutineScopes

  1. Some listeners should be added only if ViewHolder has been binded.
  2. A timer that changes TextView's text periodically should be started if Activity has been resumed, and ViewHolder has been binded.

@dovchinnikov
Copy link
Contributor

dovchinnikov commented Feb 6, 2023

In IJ we use the following function for attaching a "fake" child:

https://github.com/JetBrains/intellij-community/blob/8bce81b67d20a1fc0f538cc0d3c1c04b15b61790/platform/util/src/com/intellij/util/coroutineScope.kt#L71

It repeats the original parent-child semantics w.r.t. exception handling.

We need this to support project-level services provided by plugins. (Project x Plugin) scope should be cancelled when a project is closed, or when a plugin is unloaded. Both project and plugin scopes does not contain each other: the project can be closed without unloading of a plugin, and the plugin can be unloaded without closing the project.

Picture from internal knowledge base:
image

@elizarov
Copy link
Contributor

elizarov commented Feb 7, 2023

dovchinnikov I see that your API is based on the concept of "attaching" the scopes. This kind of method could, theoretically, be called at any time after the coroutine is launched or scope is created. However, in the example, the scope is being created and attached to parents at the same time. So, the question here is whether this is actually always the case that you know all the "parents" at the time the scope is created? It has important implications for the shape of the API.

Let's start with a simpler example, where the two scopes that need to be connected for the purpose of suspending function calls. It happens in cases like this:

class SomeService { 
    private val serviceJob = Job() // has its own lifetime represented as a Job object
    
    suspend fun doSomething() { 
         // should work _both_ inside the caller scope and inside the serviceJob
    }
}

We don't have a good solution for the need of doSomething now. If you write something like withContext(serviceJob) { ... } the the code becomes the child of serviceJob only and is not attached to the scope of the caller at all.

One potential solution to the problem that we've discussed in the past was two-pronged.

  1. Somehow warn on the code that tried to use withContext, launch and other coroutine builders with contexts that contain Job instance. The theory that the resulting code is misleading. For example, when using scope.launch(context) we want Job to be passed only via the scope parameter, while context should be Job-less. The same for withContext(context). The Job should be coming from the caller only, not from the context.

  2. Provide a separate "tag" (with TBD name, lets call it LINK_JOBS for now) to inject the secondary job into the context so that it does not override the job in the original scope, but gets attached to it. This way doSomething would look like this:

suspend fun doSomething() { 
    withContext(LINK_JOBS(serviceJob)) {
        // works _both_ inside the caller scope and inside the serviceJob
    }
}

How is this relevant to the above comment? If we follow with the above plan, then instead of the following code:

val containerScope: CoroutineScope = ...
val pluginScope: CoroutineScope = ...
val combined = CoroutineScope(SupervisorJob()).also {
    it.attachAsChildTo(containerScope)
    it.attachAsChildTo(pluginScope)
}

We could have it supported with the syntax like this:

val containerScope: CoroutineScope = ...
val pluginScope: CoroutineScope = ...
val combined = CoroutineScope(SupervisorJob() + LINKED_JBOS(containerScope.job, pluginScope.job))

What do you think?

P.S. Alternatively, we can simply access an optinal llist (or vargs) or Job instances in all our coroutine builders (launch, withContext, etc) as well as in the CoroutineScope constructor, but this solution looks less composable.

@dovchinnikov
Copy link
Contributor

dovchinnikov commented Feb 8, 2023

@elizarov thank you for considering the issue and IJ approach.

I can see that LINK_JOBS might be inconvenient to intersect scopes, just like it's currently inconvenient to create a child scope: CoroutineScope(Job(parent = parentScope.coroutineContext.job) + parentScope.coroutineContext). In IJ we don't provide a service job, but instead we inject the service with the scope:

class SomeService(
    private val serviceScope: CoroutineScope,  // injected by the IJ platform
) {
    
    suspend fun doSomething() { 
        // this looks way too wordy
        withContext(
            serviceScope.coroutineContext.minusKey(Job) + // otherwise it'd replace the job completely
            LINK_JOBS(serviceScope.coroutineContext.job),
        ) {
          ... 
        }
    }
}

I think LINK_JOBS can work for withContext, launch and async if they will ignore context Job in favour of LINK_JOBS, so it would be possible to write:

withContext(
    serviceScope.coroutineContext + // job is still there, but will be ignored
    LINK_JOBS(serviceScope.coroutineContext.job),
) {
    ... 
}

I think the path-of-least-surprise for Job(), SupervisorJob() and CompletableDeferred() APIs is to make them accept vararg parents: Job. This also makes it evident that the parents are equal, there is no such thing as main parent.

val containerScope: CoroutineScope = ...
val pluginScope: CoroutineScope = ...
val combined = CoroutineScope(SupervisorJob(containerScope.job, pluginScope.job))

@dovchinnikov
Copy link
Contributor

dovchinnikov commented Feb 8, 2023

(The rest is my speculation and brainstorm dump)

AFAIU Job(vararg parent: Job) still cannot be used in withContext, because it does not complete the job:

val intersectionJob = Job(currentCoroutineContext().job, serviceScope.coroutineContext.job)
withContext(intersectionJob) { ... }
// intersectionJob is still active 

Perhaps, a separate function (name TBD) would make sense (this is how I thought the original withContext worked):

suspend fun withContextButIntersectJobInsteadOrOverriding(ctx: CoroutineContext, block: ...) {
  val anotherJob = ctx[Job]
  if (anotherJob == null) {
    return withContext(ctx, block)
  }
  val currentJob = currentCoroutineContext()[Job]
  if (currentJob == null) {
    return withContext(ctx, block)
  }
  val intersectionJob = Job(currentJob, anotherJob) 
  try {
    return withContext(ctx + intersectionJob, block)
  }
  catch (t: Throwable) {
    intersectionJob.completeExceptionally(t)
    throw t
  }
  finally {
    intersectionJob.complete()
  }
}

withContextButIntersectJobInsteadOrOverriding(serviceScope.coroutineContext) {
  ...
}

What if the intersection would be performed on scopes? This would unlock the following possible possible API shapes:

// should perform an intersection like withContextButIntersectJobInsteadOrOverriding
// alternative name: withScope (since it brings another scope into the current scope)
suspend fun withScopeContext(scope: CoroutineScope) {}

// or, possibly, scoping extension functions
suspend fun CoroutineScope.coroutineScope() {}
suspend fun CoroutineScope.supervisorScope() {}

suspend fun doSomething() { 
    withScopeContext(serviceScope) {}
    serviceScope.coroutineScope {}
    serviceScope.supervisorScope {}
}

About context jobs

I have a gut feeling that Job is too low-level, and I'd like to see it phased out in favour of CoroutineScope.
I think passing a Job to the context of launch, async and should be considered an error because they deal with scopes. In IJ utilities I'm already enforcing this with runtime assertions [1] [2].
This, of course, will break the special case of withContext(NonCancellable) {}, but it deserves a separate top-level function anyway.

@elizarov
Copy link
Contributor

elizarov commented Mar 7, 2023

@dovchinnikov I have a gut feeling that Job is too low-level, and I'd like to see it phased out in favour of CoroutineScope.
I think passing a Job to the context of launch, async and should be considered an error because they deal with scopes.

Yes. The ideal future state is when any code that passes Job as a parameter to launch, async, etc is reported as an error. The Job should only ever appear inside a CoroutineScope. The fact that launch, async, etc even accept coroutine context with a job in it is a legacy from the pre-structured-concurrency days.

So, here is the problem: how do we carefully migrate from today's state to this ideal future state? We need to somehow start giving a warning on any code that tries to pass coroutine context with a job as a parameter to those coroutine builder functions, so that we can later turn this warning into an error. But how do we even identify such code? That is the question.

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

4 participants