fix(Turborepo): Ensure process manager stays closed #6258
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Ignored Deployments
|
🟢 Turbopack Benchmark CI successful 🟢Thanks |
🟢 CI successful 🟢Thanks |
There was a problem hiding this comment.
I think it makes sense to move the child cleanup behavior change to it's own PR. The API feels a little weird and at the very least it deserves a more involved PR review. I think having unbounded child vector growth is fine since it will always just be the size of all tasks that we execute.
| @@ -22,6 +22,7 @@ use std::{ | |||
| }; | |||
|
|
|||
| use command_group::AsyncCommandGroup; | |||
| use futures::Future; | |||
There was a problem hiding this comment.
| use futures::Future; |
|
|
||
| // just allocate a new vec rather than clearing the old one | ||
| lock.children = vec![]; | ||
| lock.is_closing = false; |
There was a problem hiding this comment.
I think we can probably just remove this line to get the prime functionality of the PR. I think the children cleaning up after themselves deserves it's own PR.
| if let Ok(child) = &child { | ||
| lock.children.push(child.clone()); | ||
| } | ||
| let inner = Arc::clone(&self.0); |
There was a problem hiding this comment.
Slightly concerned as we're creating a cycle here with the Arc getting moved into the child.
child->innervia the variables that the closure capturesinner.children[index]->child
I think we are correct as long as the closure always gets dropped which should always happen, but that is dependent on a tokio task exiting which we don't keep a handle around for.
gsoltis
force-pushed
the
gsoltis/keep_process_manager_closed
branch
from
709e6cd
to
a1f077f
Compare
|
Dropped the child cleanup piece |
Description
closehas been called, or afterwaithas completed.Testing Instructions
Updated tests.
Closes TURBO-1526