Skip to content

Fix zombie ssh processes from accumulating #1333

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 3, 2024
Merged

Fix zombie ssh processes from accumulating #1333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba93ef2
Fix zombie ssh processes from accumulating
cesfahani Apr 2, 2024
6118c5c
fix: update `h2` to 0.4.4` to avoid RUSTSEC-2024-0332
Byron Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions gix-transport/src/client/blocking_io/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,18 @@ impl client::TransportWithoutIO for SpawnProcessOnDemand {
}
}

impl Drop for SpawnProcessOnDemand {
fn drop(&mut self) {
if let Some(mut child) = self.child.take() {
// The child process (e.g. `ssh`) may still be running at this point, so kill it before joining/waiting.
// In the happy-path case, it should have already exited gracefully, but in error cases or if the user
// interrupted the operation, it will likely still be running.
child.kill().ok();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd feel better if we could remove the kill() then. Despite being aware that it would probably prevent to hang in the wait() call, I'd only want to add it when it's proven to be necessary. Since Git doesn't seem to be doing that, I think neither should we.

Copy link
Contributor Author

@cesfahani cesfahani Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed it in my most recent update. I also confirmed that with this change, my application (a long-running daemon that uses gitoxide to do tons of git clones over SSH) does not show any zombied ssh processes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I spoke too soon! Seems there's some tests where removing the kill() is causing a hang. Let me look into this...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for running this experiment!

In this case, Git might not even have all the answers as it would never encounter such a case, after all, it can't be used as a library.

Something that surprises me though is how wait() (without kill()) can still leave zombies - if it doesn't block forever then it should shut down the child for good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something that surprises me though is how wait() (without kill()) can still leave zombies - if it doesn't block forever then it should shut down the child for good.

Not sure what you mean... The issue with the test hang is that wait() is blocking forever. After adding the wait(), we are guaranteed to never have zombies (at least for this child process scenario) - we're just exposed to a deadlock possibility if the ssh process didn't shutdown gracefully.

I was able to fix the test hang with this change:

diff --git a/gix/src/remote/connection/fetch/receive_pack.rs b/gix/src/remote/connection/fetch/receive_pack.rs
index 7634b34cf..aeaf17033 100644
--- a/gix/src/remote/connection/fetch/receive_pack.rs
+++ b/gix/src/remote/connection/fetch/receive_pack.rs
@@ -253,6 +253,10 @@ where
                         .transpose()?
                         .unwrap_or(false);
                     if reject_shallow_remote {
+                        drop(reader);
+                        gix_protocol::indicate_end_of_interaction(&mut con.transport, con.trace)
+                            .await
+                            .ok();
                         return Err(Error::RejectShallowRemote);
                     }
                     shallow_lock = acquire_shallow_lock(repo).map(Some)?;

But... now that I look at it more, I feel like there's other scenarios where tke kill() is going to be needed. For example, while we're in the middle of reading the pack, the user can abort the operation via should_interrupt. If that happens, then there is no "graceful" way to indicate to the remote that we'd like to shutdown (AFAIK).

Thoughts @Byron ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I didn't know that deadlocks are actually happening then, and it makes sense to see these until the remote hangs up. Oh, and it looks like CI is already deadlocking.

And it's true, once interrupted, everything winds down and it's unclear in which state ssh is in that moment, so a kill() call would be required. Git is different, as in doesn't have that problem, as they just abort on signal. There is some special handling for tempfiles, but that's about it.

Thus, we really have to call kill here. Could you protect that kill() call with a comment that briefly explains why?

Then I think this can be merged, and we are back to were we were, but with a comment and a better understanding :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks for the feedback!

child.wait().ok();
}
}
}

struct ReadStdoutFailOnError {
recv: std::sync::mpsc::Receiver<std::io::Error>,
read: std::process::ChildStdout,
Expand Down