fix: get_children should not break supervision tree integrity #311
Conversation
ractor/src/actor/supervision.rs
Outdated
| @@ -134,15 +134,21 @@ impl SupervisionTree { | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) fn get_children(&self) -> Vec<ActorCell> { | |||
| pub(crate) fn take_children(&self) -> Vec<ActorCell> { | |||
There was a problem hiding this comment.
we shouldn't need a take method anywhere, the supervision tree should only be modified upon link/unlink. This should be as easy as just replacing the functionality inside with the non-clearing version, and updating a test case to make sure that the supervision tree isn't modified.
#310 is indeed a bug, and fixing it vs adding a new API like this is better for footgun safety.
There was a problem hiding this comment.
I think some kind of clearing is still required. Initially I just removed the clear() but then the double-link test run out of stack. I realized if we don't clear the children list after unlink, we can run into mutual recursion in the double-link scenario. So instead of calling clear() everywhere I created the take method.
|
Check out this commit. Feel free to pull it on top and update your PR |
kanru
force-pushed
the
fix-ractor-get-children
branch
from
8b061c4 to
62efc43
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 82.04% 82.07% +0.02%
==========================================
Files 61 61
Lines 12182 12183 +1
==========================================
+ Hits 9995 9999 +4
+ Misses 2187 2184 -3 ☔ View full report in Codecov by Sentry. |
This fixes #310
Please suggest where and what tests I can add.