process: kill child on error while spawning #6803
Conversation
There's a chance that between spawning of the `std Child`, and wrapping it into a tokio impl an error can occurs, which would leave the child alive, with no way to access it from the user side and not being part of the tokio runtime. This commit fixes that issue by wrapping the `std Child` right after spawning with a struct that implements Drop, where the child would be killed if it's dropped. Fixes: tokio-rs#6797
Darksonn
added
A-tokio
| @@ -115,21 +115,97 @@ impl fmt::Debug for Child { | |||
| } | |||
| } | |||
|
|
|||
| pub(crate) struct ChildDropGuard { | |||
There was a problem hiding this comment.
We already define ChildDropGuard in tokio/src/process/mod.rs. Can we:
- reuse that type instead of having to redefine it in the
unixandwindowsmodules? - re-structure how/where the
StdChildis wrapped so we don't end up double wrapping?- as this PR is currently laid out we will end up having two kill-on-drop wrappers which will have different behavior (namely trying to kill the process twice which is different behavior than we have today)
There was a problem hiding this comment.
We can maybe combine ChildDropGuard from tokio/src/process/unix/mod.rs and DroppableChild from tokio/src/process/windows.rs, and have it in tokio/src/process/mod.rs, but we cant use ChildDropGuard from tokio/src/process/mod.rs. The reason being that we want the ability to take the child out from the Wapper struct, and ChildDropGuard from tokio/src/process/mod.rs doesn't allow that. The reason being that implementing Drop doesn't allow destructuring, so in the other implementations we use Options to circumvent that.
The double wrapped child won't make the process be killed twice, as we set the inner wrapper(ChildDropGuard from tokio/src/process/unix/mod.rs) to not kill the child on drop (tokio/src/process/unix/mod.rs line 202), so only the outer one(ChildDropGuard from tokio/src/process/mod.rs) will do that if so set.
I tried to make it so there's no double wrapping, but I couldn't make the PidfdReaper::new impl work. Because I would have to some way extract the child from the wrapper in the new function. And I couldn't get it to work.
There was a problem hiding this comment.
It is okay if the definition of ChildDropGuard needs to move out of tokio/src/process/mod.rs and into the platform-specific implementation modules, but there is no need to have two wrappers which attempt to perform a kill-on-drop (and bloat the data structures with redundant bools).
There's a chance that between spawning of the
std Child, and wrappingit into a tokio impl an error can occurs, which would leave the child
alive, with no way to access it from the user side and not being part
of the tokio runtime.
This commit fixes that issue by wrapping the
std Childright afterspawning with a struct that implements Drop, where the child would
be killed if it's dropped.
Fixes: #6797