Re: [PATCH 3/3] t/README: document writing concurrency-safe helpers

[Junio C Hamano <gitster@pobox.com>]

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Michael Montalbo <mmontalbo@gmail.com>
>
> The apply-one-time-script.sh and http-429.sh fixes addressed the same
> underlying problem: a test helper assuming it has exclusive access to a
> file when the web server can run it for several requests at once. The
> atomic idioms that avoid this are not specific to CGI or to HTTP, so
> document them generally, alongside the other guidance for writing tests,
> and leave a pointer from the lib-httpd helper list rather than a local
> comment. The note covers the anti-pattern (a "test -f" then a separate
> act) and the two safe operations (mkdir to elect a winner, rename to
> consume a one-shot marker), citing Git's own lockfile machinery and
> make_symlink() as precedent.
>
> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
> ---
>  t/README       | 32 ++++++++++++++++++++++++++++++++
>  t/lib-httpd.sh |  3 +++
>  2 files changed, 35 insertions(+)

Thanks for a nice finishing touch.



> diff --git a/t/README b/t/README
> index 085921be4b..a9d425f392 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 fc646447d5..d64f9c8c2d 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