diff --git a/t/README b/t/README index 085921be4b6c2a..a9d425f392115c 100644 --- a/t/README +++ b/t/README @@ -854,6 +854,38 @@ from the test harness library. At the end of the script, call 'test_done'. +Writing concurrency-safe helpers +-------------------------------- + +Some test code runs concurrently: a test may background work with '&', +and the helper scripts installed for the web server (in t/lib-httpd) are +run once per request, so the same script can execute for several +requests at once. Such code cannot assume it has exclusive access to a +file. + +When exactly one of several concurrent processes needs to "win" a +decision, a single atomic filesystem operation can make it, rather than +a check followed by a separate action. A "test -f X" then "touch X" +(or "rm X") races: two processes can both pass the check before either +acts. Two atomic operations avoid this: + + - "mkdir dir", which fails if the directory already exists, so that + exactly one caller wins, electing a first or only request (see + t/lib-httpd/http-429.sh). + + - "mv src dst" (rename), which fails if the source is gone, so that + exactly one caller consumes it, claiming a planted one-shot marker + (see t/lib-httpd/apply-one-time-script.sh). + +A "$$" suffix on per-request scratch files keeps concurrent invocations +from clobbering each other's fixed-name files. + +This is a standard shell locking idiom, and the same reasoning behind +Git's own lockfile machinery, which creates its lock with O_CREAT|O_EXCL, +and make_symlink() in t/test-lib.sh, which uses an mkdir lock: an atomic +operation whose failure indicates that another process got there first. + + Test harness library -------------------- diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index fc646447d5c038..d64f9c8c2d0045 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -159,6 +159,9 @@ prepare_httpd() { mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH" cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH" cp "$TEST_PATH"/proxy-passwd "$HTTPD_ROOT_PATH" + # The web server can run any of these CGI scripts for two requests at + # once; a helper that keeps state between requests must do so with an + # atomic operation. See "Writing concurrency-safe helpers" in t/README. install_script incomplete-length-upload-pack-v2-http.sh install_script incomplete-body-upload-pack-v2-http.sh install_script error-no-report.sh diff --git a/t/lib-httpd/apply-one-time-script.sh b/t/lib-httpd/apply-one-time-script.sh index b1682944e280e2..a298ae89ae90cf 100644 --- a/t/lib-httpd/apply-one-time-script.sh +++ b/t/lib-httpd/apply-one-time-script.sh @@ -6,21 +6,31 @@ # # This can be used to simulate the effects of the repository changing in # between HTTP request-response pairs. -if test -f one-time-script -then - LC_ALL=C - export LC_ALL +# +# Apache can run this CGI for concurrent requests (for example a partial fetch +# that lazily fetches a missing object while the first response is still in +# flight), so the helper claims the marker atomically with a rename, and only +# once it has decided to modify the response. A request that loses the race +# finds the marker already gone and serves its response unchanged; no request +# is left emitting an empty body, which the server would report as HTTP 500. +# Scratch files are per-request ($$) so concurrent requests do not clobber each +# other. + +test -f one-time-script || exec "$GIT_EXEC_PATH/git-http-backend" - "$GIT_EXEC_PATH/git-http-backend" >out - ./one-time-script out >out_modified +LC_ALL=C +export LC_ALL - if cmp -s out out_modified - then - cat out - else - cat out_modified - rm one-time-script - fi +out=out.$$ +modified=out-modified.$$ +"$GIT_EXEC_PATH/git-http-backend" >"$out" + +if ./one-time-script "$out" 2>/dev/null >"$modified" && + ! cmp -s "$out" "$modified" && + mv one-time-script one-time-script.$$ 2>/dev/null +then + cat "$modified" else - "$GIT_EXEC_PATH/git-http-backend" + cat "$out" fi +rm -f "$out" "$modified" one-time-script.$$ diff --git a/t/lib-httpd/http-429.sh b/t/lib-httpd/http-429.sh index c97b16145b7f92..d9bbedf1ad3f2d 100644 --- a/t/lib-httpd/http-429.sh +++ b/t/lib-httpd/http-429.sh @@ -26,14 +26,17 @@ repo_path="${remaining#*/}" # Get rest (repo path) # The repo name is the first component before any "/" repo_name="${repo_path%%/*}" -# Use current directory (HTTPD_ROOT_PATH) for state file -# Create a safe filename from test_context, retry_after and repo_name -# This ensures all requests for the same test context share the same state file +# Use current directory (HTTPD_ROOT_PATH) for state. +# Create a safe name from test_context, retry_after and repo_name so that all +# requests for the same test context share the same state. safe_name=$(echo "${test_context}-${retry_after}-${repo_name}" | tr '/' '_' | tr -cd 'a-zA-Z0-9_-') -state_file="http-429-state-${safe_name}" +state="http-429-state-${safe_name}" -# Check if this is the first call (no state file exists) -if test -f "$state_file" +# Apache can run this CGI for concurrent requests, so the script decides +# whether this is the first call with a single atomic "mkdir": it succeeds for +# exactly one of any racing requests and fails for the rest. "permanent" +# always rate-limits and records no state. +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null then # Already returned 429 once, forward to git-http-backend # Set PATH_INFO to just the repo path (without retry-after value) @@ -52,9 +55,6 @@ then exec "$GIT_EXEC_PATH/git-http-backend" fi -# Mark that we've returned 429 -touch "$state_file" - # Output HTTP 429 response printf "Status: 429 Too Many Requests\r\n" @@ -67,8 +67,7 @@ case "$retry_after" in printf "Retry-After: invalid-format-123abc\r\n" ;; permanent) - # Always return 429, don't set state file for success - rm -f "$state_file" + # Always return 429 printf "Retry-After: 1\r\n" printf "Content-Type: text/plain\r\n" printf "\r\n" diff --git a/t/meson.build b/t/meson.build index 3219264fe7d497..a118a4d7196b17 100644 --- a/t/meson.build +++ b/t/meson.build @@ -707,6 +707,7 @@ integration_tests = [ 't5564-http-proxy.sh', 't5565-push-multiple.sh', 't5566-push-group.sh', + 't5567-one-time-script.sh', 't5570-git-daemon.sh', 't5571-pre-push-hook.sh', 't5572-pull-submodule.sh', diff --git a/t/t5567-one-time-script.sh b/t/t5567-one-time-script.sh new file mode 100755 index 00000000000000..cd8e6560056160 --- /dev/null +++ b/t/t5567-one-time-script.sh @@ -0,0 +1,96 @@ +#!/bin/sh + +test_description='apply-one-time-script CGI helper is safe under concurrent requests' + +. ./test-lib.sh + +HELPER="$TEST_DIRECTORY/lib-httpd/apply-one-time-script.sh" + +test_expect_success PIPE 'concurrent requests: one rewritten, one passed through, neither empty' ' + mkdir workdir fakebin && + ENTERED="$PWD/entered" && + GATE="$PWD/gate" && + export ENTERED GATE && + mkfifo "$ENTERED" "$GATE" && + + # Stand in for git-http-backend. The modify role returns a response + # containing "packfile", which the one-time script rewrites. The + # passthrough role returns a response that is left untouched, but first + # announces that it has entered the helper and then blocks, so that it + # is still in flight when the modify role claims and removes the marker. + write_script fakebin/git-http-backend <<-\EOF && + printf "Status: 200 OK\r\n" + printf "Content-Type: application/x-git-result\r\n" + printf "\r\n" + if test "$ROLE" = modify + then + printf "packfile\n" + else + echo entered >"$ENTERED" + read -r released <"$GATE" + printf "refs\n" + fi + EOF + + # The transform that replace_packfile would install as one-time-script: + # rewrite responses that contain "packfile", leave the rest alone. + write_script workdir/one-time-script <<-\EOF && + if grep packfile "$1" >/dev/null + then + sed "/packfile/q" "$1" && + printf "REPLACED\n" + else + cat "$1" + fi + EOF + + GIT_EXEC_PATH="$PWD/fakebin" && + export GIT_EXEC_PATH && + + # Hold GATE open read-write on fd 9 for the duration, so releasing the + # passthrough request below cannot block even if that request has + # already exited (it keeps a reader on the FIFO). + exec 9<>"$GATE" && + + # Launch the passthrough request in the background. It enters the + # helper, signals us through ENTERED, then blocks on GATE inside the + # fake backend. The braces keep the && chain intact while backgrounding + # only the subshell, so "wait" can reap it by pid; kill it on any exit + # so a stray blocked child cannot hold the test output open and stall a + # reader such as prove. + { ( + cd workdir && + ROLE=passthrough sh "$HELPER" >../passthrough.out 2>../passthrough.err + ) & } && + passthrough_pid=$! && + test_when_finished "kill $passthrough_pid 2>/dev/null || :" && + + # Wait until the passthrough request is past the marker check. + read -r entered <"$ENTERED" && + + # Run the modifying request to completion while the passthrough request + # is still blocked. + ( + cd workdir && + ROLE=modify sh "$HELPER" >../modify.out 2>../modify.err + ) && + + # Release the passthrough request and let it finish. Ignore the helper + # exit status here so a broken helper is diagnosed by the assertions + # below rather than aborting the test. + echo released >&9 && + { wait "$passthrough_pid" || :; } && + + # Neither request may error out or produce an empty (HTTP 500) body, + # and each must have played its role: the modify request rewrote its + # response and the passthrough request came through untouched. + test_must_be_empty passthrough.err && + test_must_be_empty modify.err && + test_grep "Status: 200 OK" passthrough.out && + test_grep "Status: 200 OK" modify.out && + test_grep REPLACED modify.out && + test_grep ! REPLACED passthrough.out && + test_grep refs passthrough.out +' + +test_done