From 6ba799c9d6b17ed665d3c352c3c4bb35c9f771bb Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 3 Jan 2023 09:46:39 +0100 Subject: [PATCH] fix: `gix clone ssh://...` won't deadlock anymore. For `cargo` specifically we now parse stderr to see if permission errors occour. This links stderr and stdout and we have to pass information from a supervisor thread that parses stderr to stdout and use the information to return a custom io error in time. Now the algorithm is adjusted to never be able to deadlock, as the problem is inherently racy and somewhat hard to implement it properly especially without a good test suite built-into `gitoxde` - there are no ssh servers one can easily spin up cross-platform. --- git-transport/src/client/blocking_io/file.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/git-transport/src/client/blocking_io/file.rs b/git-transport/src/client/blocking_io/file.rs index 6a63c4bf672..2fa6d5ad2b1 100644 --- a/git-transport/src/client/blocking_io/file.rs +++ b/git-transport/src/client/blocking_io/file.rs @@ -138,7 +138,23 @@ fn supervise_stderr( Some(err) => Err(err), None => match res { Ok(n) if n == wanted => Ok(n), - Ok(n) => self.recv.recv().ok().map(Err).unwrap_or(Ok(n)), + Ok(n) => { + // TODO: fix this + // When parsing refs this seems to happen legitimately + // (even though we read packet lines only and should always know exactly how much to read) + // Maybe this still happens in `read_exact()` as sometimes we just don't get enough bytes + // despite knowing how many. + // To prevent deadlock, we have to set a timeout which slows down legitimate parts of the protocol. + // This code was specifically written to make the `cargo` test-suite pass, and we can reduce + // the timeouts even more once there is a native ssh transport that is used by `cargo`, it will + // be able to handle these properly. + // Alternatively, one could implement something like `read2` to avoid blocking on stderr entirely. + self.recv + .recv_timeout(std::time::Duration::from_millis(5)) + .ok() + .map(Err) + .unwrap_or(Ok(n)) + } Err(err) => Err(self.recv.recv().ok().unwrap_or(err)), }, } @@ -153,7 +169,7 @@ fn supervise_stderr( let (send, recv) = std::sync::mpsc::sync_channel(1); std::thread::Builder::new() - .name("supervise ssh".into()) + .name("supervise ssh stderr".into()) .stack_size(128 * 1024) .spawn(move || -> std::io::Result<()> { let mut process_stderr = std::io::stderr();