Skip to content

Commit 10e0840

Browse files
authored
Merge pull request #312 from Gitlawb/fix/shim-nonblocking-accept
fix(git-remote): normalize the shim's accepted socket to blocking
2 parents 241b366 + d4e5ecf commit 10e0840

1 file changed

Lines changed: 236 additions & 1 deletion

File tree

‎crates/git-remote-gitlawb/tests/real_git_fetch.rs‎

Lines changed: 236 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,8 +162,28 @@ fn start_shim(repo: PathBuf, mode: ShimMode) -> Shim {
162162
}
163163
}
164164

165-
fn handle_conn(stream: TcpStream, repo: &Path, mode: ShimMode, posts: &AtomicUsize) {
165+
/// `start_shim` makes the LISTENER non-blocking to poll its stop flag, and an
166+
/// accepted socket can inherit that flag.
167+
///
168+
/// Observed on Windows: Winsock hands back a socket carrying the listener's
169+
/// FIONBIO state and std's `accept` does not reset it. Expected on macOS and the
170+
/// BSDs for the same reason, since the accept4 man page describes inheritance as
171+
/// the canonical BSD behavior, though nothing in this repo's CI exercises that
172+
/// half. Linux is the exception, because std uses `accept4` there and it does
173+
/// not inherit file status flags (rust-lang/rust#67027).
174+
///
175+
/// Every read in `handle_conn` is a blocking read whose errors collapse to "peer
176+
/// closed", so an inherited WouldBlock would drop the connection with no
177+
/// response and abort the client mid-request. Normalize before reading; this
178+
/// also restores the read timeout, which a non-blocking socket silently makes
179+
/// meaningless.
180+
fn normalize_accepted_stream(stream: &TcpStream) {
181+
stream.set_nonblocking(false).ok();
166182
stream.set_read_timeout(Some(Duration::from_secs(30))).ok();
183+
}
184+
185+
fn handle_conn(stream: TcpStream, repo: &Path, mode: ShimMode, posts: &AtomicUsize) {
186+
normalize_accepted_stream(&stream);
167187
let mut reader = BufReader::new(stream);
168188

169189
// Request line.
@@ -749,6 +769,221 @@ fn build_divergent_repos(shared_commits: usize) -> Repos {
749769
}
750770
}
751771

772+
/// `normalize_accepted_stream` must leave the socket in blocking mode.
773+
///
774+
/// This asserts the helper's postconditions directly: no client, no request, and
775+
/// no race over whether bytes arrive before the shim reads them. It removes the
776+
/// byte-arrival race entirely, which is what the end-to-end test cannot do. It is
777+
/// not literally deterministic, and the paragraph below says exactly what is left.
778+
///
779+
/// The read timeout has a getter, so it is checked exactly. Blocking mode does
780+
/// not: std exposes none, and Winsock has no FIONBIO getter at all (`ioctlsocket`
781+
/// documents FIONBIO with no output type, so it is set-only), so that half has to
782+
/// be observed through behavior: a blocking socket with no data pending parks its
783+
/// reader, a non-blocking one returns `WouldBlock` at once.
784+
///
785+
/// The reader owns the very socket the helper normalized. Do not reintroduce a
786+
/// `try_clone` here: on Windows that mints a new descriptor through
787+
/// `WSADuplicateSocketW` rather than duplicating a handle, and whether FIONBIO
788+
/// state follows the new descriptor is a question this test would then depend on
789+
/// and could not answer. Owning the original removes the question.
790+
///
791+
/// The `Started` handshake is load-bearing and must not be removed. It SHRINKS
792+
/// the vacuity window; it does not close it. Read that literally, because the
793+
/// looser version of this sentence is the mistake this whole test exists to
794+
/// correct. Without the handshake the window includes thread spawn itself, which
795+
/// is unbounded and can be very slow on a loaded runner. With it, spawn is out of
796+
/// the window entirely and what remains is the cross-thread round trip: the
797+
/// signal, the read call, its `WouldBlock` return, the channel send, and this
798+
/// thread waking from `recv_timeout`.
799+
///
800+
/// So the residual false-green path is a reader descheduled anywhere along that
801+
/// round trip for more than 250ms, notably between its own `Started` send and its
802+
/// `read`. That path is real and this comment does not claim otherwise. What the
803+
/// design does buy is that the guard cannot flake RED, and that no arrangement of
804+
/// bytes on the wire can fool it, only the scheduler. Measured 50 of 50 RED under
805+
/// a mutation deleting the normalization, at full-suite parallelism; that bounds
806+
/// the false-green rate, it does not make it zero.
807+
///
808+
/// Teardown order is a constraint, not a preference: the peer close must come
809+
/// AFTER the timeout assertion. Closing first unblocks the parked read, the
810+
/// outcome arrives, and the assertion fails even when the helper is correct.
811+
#[test]
812+
fn normalize_accepted_stream_leaves_the_socket_blocking() {
813+
enum Probe {
814+
Started,
815+
Outcome(std::io::Result<usize>),
816+
}
817+
818+
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
819+
let addr = listener.local_addr().unwrap();
820+
let client = TcpStream::connect(addr).unwrap();
821+
let (accepted, _) = listener.accept().unwrap();
822+
823+
// The state Windows hands the shim. Forced here so the property is checked
824+
// on every platform rather than only where the OS supplies it.
825+
accepted.set_nonblocking(true).unwrap();
826+
normalize_accepted_stream(&accepted);
827+
828+
// The helper's other postcondition, and the one std lets us check exactly.
829+
// Without this, deleting the timeout leaves every test here green while a
830+
// stalled peer parks the shim's only accept-loop thread forever.
831+
assert_eq!(
832+
accepted.read_timeout().unwrap(),
833+
Some(Duration::from_secs(30)),
834+
"normalize_accepted_stream must restore the read timeout, not only blocking mode"
835+
);
836+
837+
let (tx, rx) = std::sync::mpsc::channel();
838+
// Move, never clone: see the try_clone note above.
839+
let mut reader = accepted;
840+
let probe = std::thread::spawn(move || {
841+
let mut buf = [0u8; 1];
842+
tx.send(Probe::Started).unwrap();
843+
let outcome = reader.read(&mut buf);
844+
let _ = tx.send(Probe::Outcome(outcome));
845+
});
846+
847+
// Bounded, not `recv()`. A bare recv here turns "the reader never ran" into a
848+
// hung test binary rather than a failing test, and cargo test has no per-test
849+
// timeout to rescue it. The bound is far above any real scheduling delay, so
850+
// hitting it is a genuine defect report, never a flake.
851+
match rx
852+
.recv_timeout(Duration::from_secs(30))
853+
.expect("reader thread never signalled Started (died, or never scheduled)")
854+
{
855+
Probe::Started => {}
856+
Probe::Outcome(_) => panic!("reader reported an outcome before its Started signal"),
857+
}
858+
859+
match rx.recv_timeout(Duration::from_millis(250)) {
860+
// The read is still parked, which is what blocking mode means.
861+
Err(std::sync::mpsc::RecvTimeoutError::Timeout) => {}
862+
Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => {
863+
panic!("the reader thread exited instead of parking in its read")
864+
}
865+
Ok(Probe::Started) => panic!("received a second Started signal"),
866+
Ok(Probe::Outcome(outcome)) => {
867+
assert_eq!(
868+
outcome.as_ref().err().map(std::io::Error::kind),
869+
Some(std::io::ErrorKind::WouldBlock),
870+
"the read returned instead of parking, but not with WouldBlock \
871+
({outcome:?}); that is a different failure than the one this test \
872+
guards, so do not read it as a non-blocking socket"
873+
);
874+
panic!(
875+
"accepted socket is still non-blocking after \
876+
normalize_accepted_stream: the read returned WouldBlock \
877+
immediately instead of parking"
878+
);
879+
}
880+
}
881+
882+
// Only now: EOF unblocks the parked read so the thread can be joined. Bounded
883+
// for the same reason as the handshake wait above; the socket's own 30s read
884+
// timeout is the other backstop, and it is asserted rather than assumed.
885+
drop(client);
886+
match rx
887+
.recv_timeout(Duration::from_secs(60))
888+
.expect("reader did not return after the peer closed")
889+
{
890+
Probe::Outcome(outcome) => assert_eq!(
891+
outcome.ok(),
892+
Some(0),
893+
"expected EOF once the peer closed, not payload"
894+
),
895+
Probe::Started => panic!("received a second Started signal"),
896+
}
897+
probe.join().unwrap();
898+
}
899+
900+
/// The shim must answer a connection that arrives in non-blocking mode.
901+
///
902+
/// End-to-end realism check through the real `handle_conn` request path. The
903+
/// deterministic guarantee lives in
904+
/// `normalize_accepted_stream_leaves_the_socket_blocking`, which asserts the
905+
/// helper's postcondition with no client at all; see `normalize_accepted_stream`
906+
/// for why an accepted socket can arrive non-blocking in the first place. The
907+
/// shim reads with `read_line(..).unwrap_or(0)`, which collapses `WouldBlock`
908+
/// into "peer closed" and returns WITHOUT writing a response, aborting the
909+
/// client mid-request.
910+
///
911+
/// This test is BEST EFFORT and known to be nondeterministic. Its handshake
912+
/// fires after `accept()` but before `handle_conn` runs, so it narrows the
913+
/// accept-before-request window without closing it: the request bytes can
914+
/// already be buffered by the time the shim reads, and the read then succeeds
915+
/// even with the normalization removed. Against a deleted
916+
/// `set_nonblocking(false)` it caught the defect in 39, 43, and 47 of 50 across
917+
/// three full-suite samples. The spread is the point: it is a sampled tendency,
918+
/// not a property, so do not quote a percentage off it. Running it alone on an
919+
/// idle machine flattered it to 48 of 50; the full-suite numbers are the honest
920+
/// ones, because that is how it ships.
921+
///
922+
/// It is still the only coverage for the helper's CALL SITE. The deterministic
923+
/// probe calls `normalize_accepted_stream` directly, so deleting the call from
924+
/// `handle_conn` leaves that probe green (0 of 50 in both samples) and only this
925+
/// test notices, at 45 and 46 of 50.
926+
#[test]
927+
fn shim_answers_a_connection_that_arrives_non_blocking() {
928+
let repos = build_divergent_repos(1);
929+
let repo = repos.server.clone();
930+
931+
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
932+
listener.set_nonblocking(true).unwrap();
933+
let addr = listener.local_addr().unwrap();
934+
let (accepted_tx, accepted_rx) = std::sync::mpsc::channel();
935+
936+
let server = std::thread::spawn(move || {
937+
let posts = AtomicUsize::new(0);
938+
let stream = loop {
939+
match listener.accept() {
940+
Ok((s, _)) => break s,
941+
Err(ref e) if e.kind() == std::io::ErrorKind::WouldBlock => {
942+
std::thread::sleep(Duration::from_millis(5))
943+
}
944+
Err(e) => panic!("accept failed: {e}"),
945+
}
946+
};
947+
// What Windows hands the shim, and what macOS is expected to. On Linux
948+
// this is the only way to reach that state, so forcing it is what makes
949+
// the assertions below run on every platform.
950+
stream.set_nonblocking(true).unwrap();
951+
accepted_tx.send(()).unwrap();
952+
handle_conn(stream, &repo, ShimMode::Normal, &posts);
953+
});
954+
955+
let mut client = TcpStream::connect(addr).unwrap();
956+
// Send only once the shim has the connection in hand, so its first read is
957+
// guaranteed to run against an empty receive buffer.
958+
accepted_rx.recv().unwrap();
959+
client
960+
.write_all(b"GET /x/info/refs?service=git-upload-pack HTTP/1.1\r\nHost: x\r\n\r\n")
961+
.unwrap();
962+
963+
let mut got = Vec::new();
964+
let read = client.read_to_end(&mut got);
965+
server.join().unwrap();
966+
967+
assert!(
968+
read.is_ok(),
969+
"client read failed instead of receiving a response: {:?} \
970+
(a non-blocking accepted socket makes the shim drop the connection \
971+
with no reply, which surfaces as a connection abort on the client)",
972+
read.unwrap_err().kind()
973+
);
974+
assert!(
975+
got.starts_with(b"HTTP/1.1 200 OK"),
976+
"shim closed the connection without a response ({} bytes read); \
977+
a WouldBlock on the request-line read was swallowed as EOF",
978+
got.len()
979+
);
980+
assert!(
981+
got.windows(b"# service=git-upload-pack".len())
982+
.any(|w| w == b"# service=git-upload-pack"),
983+
"response reached the client but is not the upload-pack advertisement"
984+
);
985+
}
986+
752987
/// Matrix item 7, bridging half: a real multi-round `git fetch` through the
753988
/// helper against a real `git upload-pack --stateless-rpc` completes, is observed
754989
/// at >=2 POSTs (the executable form of the trigger; a fixture that resolved in

0 commit comments

Comments
 (0)