Re: [RFC PATCH v1 0/6] stash: drop usage of a second index

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Mon, Jun 15, 2020 at 11:50:20PM +0200, SZEDER Gábor wrote:
> On Mon, Jun 15, 2020 at 05:27:15PM +0200, SZEDER Gábor wrote:
> >       - Should we even allow 'splitIndex.sharedIndexExpire=now'?
> >     
> >         I believe, though haven't confirmed, that it can cause trouble
> >         even without using an alternate index.  Consider the following
> >         sequence of events:
> >     
> >           - Git process A reads '.git/index', finds the 'link' extension,
> >             and reads the SHA1 recorded there that determines the filename
> >             of its shared index.
> >     
> >           - The scheduler steps in, and puts process A to sleep.
> >     
> >           - Git process B updates the index, decides that it's time to
> >             write a new shared index, does so, and then because of
> >             'splitIndex.sharedIndexExpire=now' it removes all other shared
> >             index files.
> >     
> >           - The scheduler wakes process A, which now tries to open the
> >             shared index file it just learned about, but fails because
> >             that file has just been removed by process B.
> 
> Confirmed.
> 
> To help reproduce the issue, this diff adds a strategically-placed
> controllable delay between reading '.git/index' and reading its
> shared/base index:
> 
> diff --git a/read-cache.c b/read-cache.c
> index b888c5df44..5a66e9bf4b 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2319,6 +2319,9 @@ int read_index_from(struct index_state *istate, const char *path,
>  	else
>  		split_index->base = xcalloc(1, sizeof(*split_index->base));
>  
> +	if (git_env_bool("GIT_TEST_WAIT", 0))
> +		sleep(3);

We don't even need this sleep() call ...

>  	base_oid_hex = oid_to_hex(&split_index->base_oid);
>  	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
>  	trace2_region_enter_printf("index", "shared/do_read_index",
> 
> 
> Then this test creates the above described sequence of events:
> 
> test_expect_failure 'splitIndex.sharedIndexExpire=now can be harmful' '
> 	>file1 &&
> 	>file2 &&
> 	git update-index --split-index --add file1 &&
> 
> 	{
> 		sleep 1 &&

... and this 'sleep 1' command.

> 		# "process B"
> 		git -c splitIndex.sharedIndexExpire=now \
> 		    update-index --split-index --add file2 &
> 	} &&
> 
> 	# "process A"
> 	GIT_TEST_WAIT=1 git diff --cached --name-only
> '
> 
> ... and fails reliably with:
> 
>   [...]
>   + GIT_TEST_WAIT=1 git diff --cached --name-only
>   [ ... trace from background commands removed ...]
>   fatal: .git/sharedindex.818f65852e7402f236aeaadd32efdbb62291aa75: index file open failed: No such file or directory

Flipping the test to 'test_expect_success' and running it with
'--stress' almost always fails within 2 seconds.


> >         This is similar to the issue we have with 'git gc --prune=now',
> >         except that 'git gc's documentation explicitly warns about the
> >         risks of using '--prune=now', while the description of
> >         'splitIndex.sharedIndexExpire' doesn't have any such warning.
> >     
> >         I think that 'splitIndex.sharedIndexExpire=now' should be allowed,
> >         for those who hopefully know what they are doing, just as we allow
> >         'git gc --prune=now', but the documentation should clearly warn
> >         against its potential pitfalls.