-
-
Notifications
You must be signed in to change notification settings - Fork 770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kill and reap ProxyCommand #5494
Conversation
19d3bd2
to
a2fa5b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
I have a slightly different proposal; would you mind updating to match it?
wezterm-ssh/Cargo.toml
Outdated
@@ -39,6 +39,7 @@ wezterm-uds = { path = "../wezterm-uds" } | |||
|
|||
# Not used directly, but is used to centralize the openssl vendor feature selection | |||
async_ossl = { path = "../async_ossl" } | |||
scopeguard = "1.2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while scopeguard is handy in a pinch, I'm not in favor of it here for a couple of reasons:
- It is an additional external dependency
- You still have to remember to initiate the scope guard, and that is required in multiple places in this PR
What I'd prefer to see as a solution for this is for connect_to_host
to return a little wrapper around the Child:
struct KillOnDropChild(Child);
impl std::ops::Deref for KillOnDropChild { ... }
impl std::ops::DerefMut for KillOnDropChild { ... }
impl Drop for KillOnDropChild {
fn drop(&mut self) {
self.0.kill();
self.0.wait();
}
}
Sounds reasonable, I'll update with the requested changes. |
a2fa5b7
to
9c09369
Compare
Updated, I think this does the trick without use of |
The wrapper struct ensures ensures the child process spawned for the ProxyCommand is cleaned up under all the various error scenarios (such as auth failures etc), as well as in the normal successful use case. This includes killing it if necessary, and then waiting for it.
9c09369
to
73c332e
Compare
Thank you! |
Using the scopeguard crate ensures the child process (if any) spawned for the ProxyCommand is cleaned up under all the various error scenarios (such as auth failures etc), as well as in the normal successful use case. This ensures killing it if necessary, and then waiting for it.
This is a fix for #5479.