[runtime] ensure child supervision survives pre-spawn context cloning #1850
Conversation
Deploying monorepo with
|
| Latest commit: |
902d0e1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://adb0a1d1.monorepo-eu0.pages.dev |
| Branch Preview URL: | https://andre-context-dependency-tre.monorepo-eu0.pages.dev |
Does this implicitly enforce |
runtime/src/tokio/runtime.rs
Outdated
| // children tree, any clones created before this call (for example, actors built | ||
| // during initialization) also retarget to `children`. The previous list is kept | ||
| // so we can link this task back to its parent. | ||
| let previous = self.children.replace(children.clone()); |
There was a problem hiding this comment.
Can you add a test that this still works if spawns are called in the wrong order (child before parent)?
There was a problem hiding this comment.
We should also add a test for the case that some intermediate layer is never spawned (this could happen if there is an extra clone on the path?)
|
As discussed, a simplifying technique for this problem could be storing the task tree in executor rather than in each context (this ensures any tasks spawned in the heirarchy will be aborted, even if there are gaps). If we are supervised and end up getting called (after some parent has been aborted), we can just not schedule the inner future. |
| // while the parent keeps its original abort handles. The returned triple provides | ||
| // the optional parent supervisor list (if the child is supervised), the new child's | ||
| // aborter list, and the branch that should be installed on the child context. | ||
| let (parent_children, children, child_children) = |
There was a problem hiding this comment.
Could probably do with better names for these variables.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1850 +/- ##
=======================================
Coverage 92.19% 92.20%
=======================================
Files 305 305
Lines 79457 79538 +81
=======================================
+ Hits 73255 73335 +80
- Misses 6202 6203 +1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| struct ChildrenInner { | ||
| /// Abort handles for tasks spawned directly under this branch. Whenever the branch | ||
| /// completes or is aborted we drain this list and cancel descendants. | ||
| owned: Arc<Mutex<Vec<Aborter>>>, |
There was a problem hiding this comment.
If we have Mutex<> around ChildrenInner, why do we need one here?
There was a problem hiding this comment.
I suppose its because we destructure ChildrenInner and pass it around.
2 participants
Cloning a runtime context before its parent task spawned captured the parent's aborter list, so aborting the parent left those earlier clones running. This PR introduces a
Childrenhelper that keeps aborter lists in sync across clones: eachContext::clonebranches the dependency tree, and spawn propagates the fresh list to every branch before the task starts. Both deterministic and tokio runtimes now use this helper, ensuring abort cascades stay correct even when contexts are cloned ahead of time.Related #1849
Related #1666 - With this PR the tests there pass without any changes (I promise!)