Skip to content

Commit

Permalink
Fix a deadlocking test with master libgit2
Browse files Browse the repository at this point in the history
This commit fixes a test in Cargo to work around a seeming regression in
behavior in libgit2 around HTTP authentication. The expected flow for
HTTP authentication with git is that git sends an HTTP request and
receives an "unauthorized" response. It then sends another request with
authorization information and that's what we're testing is received in
the our test.

Previously libgit2 would issue a new HTTP connection if the previous one
was closed, but it looks like changes in libgit2 now require that the
same HTTP connection is used for the initial request and the subsequent
request with authorization information. This broke our test since it's
not using an HTTP compliant server at all and is just some handwritten
TCP reads/writes. The fix here is to basically stay with handwritten TCP
reads/writes but tweak how it happens so it's all on the same HTTP/TCP
connection to match what libgit2 is expecting.

Some extra assertions have also been added to try to prevent deadlocks
from happening in the future and instead make the test fail fast if this
situation comes up again.
  • Loading branch information
alexcrichton committed Jul 25, 2019
1 parent caba711 commit bd7fe89
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 14 deletions.
1 change: 0 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ features = [
]

[dev-dependencies]
bufstream = "0.1"
cargo-test-macro = { path = "crates/cargo-test-macro", version = "0.1.0" }

[[bin]]
Expand Down
33 changes: 20 additions & 13 deletions tests/testsuite/build_auth.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
use std;
use std::collections::HashSet;
use std::io::prelude::*;
use std::io::BufReader;
use std::net::TcpListener;
use std::sync::atomic::{AtomicUsize, Ordering::SeqCst};
use std::sync::Arc;
use std::thread;

use crate::support::paths;
use crate::support::{basic_manifest, project};
use bufstream::BufStream;
use git2;

// Tests that HTTP auth is offered from `credential.helper`.
Expand All @@ -25,15 +26,20 @@ fn http_auth_offered() {
.collect()
}

let connections = Arc::new(AtomicUsize::new(0));
let connections2 = connections.clone();
let t = thread::spawn(move || {
let mut conn = BufStream::new(server.accept().unwrap().0);
let mut conn = BufReader::new(server.accept().unwrap().0);
let req = headers(&mut conn);
conn.write_all(
b"HTTP/1.1 401 Unauthorized\r\n\
connections2.fetch_add(1, SeqCst);
conn.get_mut()
.write_all(
b"HTTP/1.1 401 Unauthorized\r\n\
WWW-Authenticate: Basic realm=\"wheee\"\r\n\
Content-Length: 0\r\n\
\r\n",
)
.unwrap();
)
.unwrap();
assert_eq!(
req,
vec![
Expand All @@ -44,16 +50,16 @@ fn http_auth_offered() {
.map(|s| s.to_string())
.collect()
);
drop(conn);

let mut conn = BufStream::new(server.accept().unwrap().0);
let req = headers(&mut conn);
conn.write_all(
b"HTTP/1.1 401 Unauthorized\r\n\
connections2.fetch_add(1, SeqCst);
conn.get_mut()
.write_all(
b"HTTP/1.1 401 Unauthorized\r\n\
WWW-Authenticate: Basic realm=\"wheee\"\r\n\
\r\n",
)
.unwrap();
)
.unwrap();
assert_eq!(
req,
vec![
Expand Down Expand Up @@ -144,6 +150,7 @@ Caused by:
))
.run();

assert_eq!(connections.load(SeqCst), 2);
t.join().ok().unwrap();
}

Expand Down

0 comments on commit bd7fe89

Please sign in to comment.