Fix coroutine scoping of Redwood Treehouse UI #1932
Conversation
| @@ -121,9 +130,12 @@ private class RedwoodZiplineTreehouseUi( | |||
| } | |||
|
|
|||
| override fun close() { | |||
| coroutineScope.coroutineContext.job.invokeOnCompletion { | |||
| @@ -55,6 +59,11 @@ private class RedwoodZiplineTreehouseUi( | |||
| */ | |||
| override val scope = (treehouseUi as? ZiplineScoped)?.scope ?: ZiplineScope() | |||
|
|
|||
| private val coroutineScope = CoroutineScope( | |||
| appLifecycle.coroutineScope.coroutineContext + | |||
| SupervisorJob(appLifecycle.coroutineScope.coroutineContext.job), | |||
There was a problem hiding this comment.
Why this change? Doesn't this mean crashes in the composition won't correctly propagate?
There was a problem hiding this comment.
As far as I can tell the unhandled exceptions still propagate up to the appLifecycle scope's job and are handled by its exception handler--but you're right, a supervisor job is probably not correct here.
We just need some per-UI scope that can be cancelled independently of the app's overall scope.
There was a problem hiding this comment.
Yeah sorry the "this" was meant solely to be about the SupervisorJob. The subscope makes sense 👍
|
Also if you're fixing something please add a change log entry to accompany it. |
dellisd
force-pushed
the
derekellis/2024-04-03/zipline-flows
branch
2 times, most recently
from
af273db to
efcb566
Compare
| private val coroutineScope = CoroutineScope( | ||
| appLifecycle.coroutineScope.coroutineContext + | ||
| appLifecycle.coroutineScope.coroutineContext.job, | ||
| ) |
There was a problem hiding this comment.
| private val coroutineScope = CoroutineScope( | |
| appLifecycle.coroutineScope.coroutineContext + | |
| appLifecycle.coroutineScope.coroutineContext.job, | |
| ) | |
| private val coroutineScope = CoroutineScope( | |
| appLifecycle.coroutineScope.coroutineContext + | |
| Job(appLifecycle.coroutineScope.coroutineContext.job), | |
| ) |
I think you probably still want to put in a new Job which gets canceled. Or does creating a new CoroutineScope do that automatically? If so, you probably don't need the + ... part.
There was a problem hiding this comment.
Oh woops, that's right. A new Job is only created if there's no Job in the context. This scope's job should be a child of the parent scope's job, but this is giving both scopes the same job. Will fix!
Ensure that Zipline resources aren't closed until the UI's coroutine scope job completes
dellisd
force-pushed
the
derekellis/2024-04-03/zipline-flows
branch
from
efcb566 to
bfc07fc
Compare
Ensure that Zipline resources aren't closed until the UI's coroutine scope job completes.
This prevents a queued-up job from resuming(?) and crashing from a
ZiplineExceptioninstead of gracefully handling aCancellationExceptionwhen trying to collect aZiplineService-backed flow.CHANGELOG.md's "Unreleased" section has been updated, if applicable.