Skip to content

Conversation

tKe
Copy link
Collaborator

@tKe tKe commented Sep 25, 2025

I'm not entirely certain if there's some hidden nuance on these, but the intent is to provide "unwrapped" coroutine scopes bound to the resourceScope lifecycle that could then be passed/composed into other resources.

@tKe tKe requested review from serras, nomisRev and kyay10 September 25, 2025 10:21
Copy link
Collaborator

@kyay10 kyay10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful! I believe Simon had something similar in mind in #3443.

Comment on lines 550 to 561
public fun CompletableJob.completeWith(exitCase: ExitCase) {
when (exitCase) {
is ExitCase.Cancelled -> cancel(exitCase.exception)
is ExitCase.Failure -> completeExceptionally(exitCase.failure)
ExitCase.Completed -> complete()
}
}

public suspend fun CompletableJob.completeWithAndJoin(exitCase: ExitCase) {
completeWith(exitCase)
return join()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use case of those functions? I'm a bit worried about making the API too big.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the only real use-case is if you had a CompletableJob from elsewhere you wanted to complete in an onRelease block or such.

It probably makes sense to make it private for now. I left it public as it felt useful for composition, but I can't see many reasons to create a completable job that aren't already covered by the scopes in this PR.

I did consider additionally adding something like

public fun ResourceScope.cancelAndJoinOnRelease(job: Job) {
  onRelease { exitCase ->
    when(exitCase) {
      is ExitCase.Cancelled -> job.cancel(exitCase.exception)
      is ExitCase.Failure -> job.cancel("Resource scope failure", exitCase.failure)
      ExitCase.Completed -> job.cancel()
    }
    job.join()
  }
}

which would cover tying any job lifecycle (for example, kafka/channel consumer, flow producer) into the resource scope, which I've used as a pattern in a number of services. It's simple to do as a local extension in each service but seems like something that might be available out of the box for resource management.

I'll mark the completeWith functions as private for now, and if viable raise another PR for the cancelAndJoinOnRelease function for discussion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyay10 do you see any value in keeping the completeWith and completeWithAndJoin helpers public?

Comment on lines 557 to 560
private suspend fun CompletableJob.completeWithAndJoin(exitCase: ExitCase) {
completeWith(exitCase)
return join()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a style thing, but may you (literally) inline this function into its (single) usage below. I think that it would made for a simpler codebase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure - I split it to mirror Job's cancel and cancelWithJoin but as it's not exposed now there's no benefit.


private inline fun ResourceScope.coroutineScope(coroutineContext: CoroutineContext, jobCreator: (Job?) -> CompletableJob): CoroutineScope {
val job = jobCreator(coroutineContext[Job])
onRelease { job.completeWithAndJoin(it) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
onRelease { job.completeWithAndJoin(it) }
onRelease { exitCase ->
job.completeWith(exitCase)
job.join()
}

Comment on lines 556 to 569
private inline fun ResourceScope.coroutineScope(coroutineContext: CoroutineContext, jobCreator: (Job?) -> CompletableJob): CoroutineScope {
val job = jobCreator(coroutineContext[Job])
onRelease { exitCase ->
job.completeWith(exitCase)
job.join()
}
return CoroutineScope(coroutineContext + job)
}

public fun ResourceScope.supervisorScope(coroutineContext: CoroutineContext = EmptyCoroutineContext): CoroutineScope =
coroutineScope(coroutineContext, ::SupervisorJob)

public fun ResourceScope.coroutineScope(coroutineContext: CoroutineContext = EmptyCoroutineContext): CoroutineScope =
coroutineScope(coroutineContext, ::Job)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coroutineScope and supervisorScope functions use continuation style. It think we should follow the same pattern, or change their names, to avoid surprises.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion is to use something like managedCoroutineScope, acquireCoroutineScope, or something along those lines.

Copy link
Collaborator Author

@tKe tKe Sep 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was relying on the difference between passing a lambda or not. I also questioned whether they should inherit the currentCoroutineContext() by default as coroutineScope{} does but decided these are analogous of the CoroutineScope() "constructor" and should therefore imply no default context (you can always use coroutineScope(coroutineContext) as needed).

Maybe managed prefix makes sense though as it's otherwise not as easy to distinguish either by lack of lambda or lack of capitalisation alone.

return CoroutineScope(coroutineContext + job)
}

public fun ResourceScope.managedSupervisorScope(coroutineContext: CoroutineContext = EmptyCoroutineContext): CoroutineScope =
Copy link
Collaborator

@kyay10 kyay10 Sep 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion. I'm worried this breaks structured concurrency because it doesn't automatically inherit the current coroutine scope. Maybe a contextual version of it could be nice to have? As in, it takes in CoroutineScope as a context, and uses it as the basis for its context, with extras passed in as the argument. This is analogous to coroutineScope and supervisorScope, but maybe there's something I'm missing here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intended this to be analogous to the CoroutineScope "constructor" which doesn't automatically inherit context, but with its lifecycle coupled to the containing resource scope.

Given the nature of resource scope, structured concurrency should be maintained even without the job inheritance.

One could always passcurrentCoroutineContext() if truly desired.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood the use case here then. I would suggest to emphasize the "constructorness" by naming it ManagedCoroutineScope instead. Given the potential for confusion, I also suggest to add an OptIn annotation, and describe in the documentation the expected use case.

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

Successfully merging this pull request may close these issues.

3 participants