Re: [PATCH v2 10/17] lock_ref_sha1_basic(): on SCLD_VANISHED, retry

[Michael Haggerty <mhagger@alum.mit.edu>]

On 01/06/2014 06:54 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> If safe_create_leading_directories() fails because a file along the
>> path unexpectedly vanished, try again (up to 3 times).
>>
>> This can occur if another process is deleting directories at the same
>> time as we are trying to make them.  For example, "git pack-refs
>> --all" tries to delete the loose refs and any empty directories that
>> are left behind.  If a pack-refs process is running, then it might
>> delete a directory that we need to put a new loose reference in.
>>
>> If safe_create_leading_directories() thinks this might have happened,
>> then take its advice and try again (maximum three attempts).
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  refs.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 3926136..6eb8a02 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2039,6 +2039,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>>  	int type, lflags;
>>  	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
>>  	int missing = 0;
>> +	int attempts = 3;
>>  
>>  	lock = xcalloc(1, sizeof(struct ref_lock));
>>  	lock->lock_fd = -1;
>> @@ -2093,7 +2094,15 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
>>  	if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
>>  		lock->force_write = 1;
>>  
>> -	if (safe_create_leading_directories(ref_file)) {
>> + retry:
>> +	switch (safe_create_leading_directories(ref_file)) {
>> +	case SCLD_OK:
>> +		break; /* success */
>> +	case SCLD_VANISHED:
>> +		if (--attempts > 0)
>> +			goto retry;
>> +		/* fall through */
> 
> Hmph.
> 
> Having no backoff/sleep at all might be OK here as long as the other
> side that removes does not retry (and I do not think the other side
> would, even though I haven't read through the series to the end yet
> ;-)).

remove_dir_recurse() only tries deleting directories once (I haven't
changed that).  And from a broader perspective, it would be pretty silly
for any tidy-up-directories function to try deleting things more than
once.  So I don't think it is a problem.  But even in the worst case,
this function only tries three times before giving up, so it shouldn't
be a disaster.

> This may be just a style thing, but I find that the variable name
> "attempts" that starts out as 3 quite misleading, as its value is
> not "the number of attempts made" but "the remaining number of
> attempts allowed."  Starting it from 0 and then
> 
> 	if (attempts++ < MAX_ATTEMPTS)
> 		goto retry;
> 
> would be one way to clarify it.  Renaming it to remaining_attempts
> would be another.

I just renamed the variable to attempts_remaining.  (I thought I was
following your suggestion, but now I see that I put the words in the
opposite order; oh well, I think it's fine either way.)

Thanks for your review!  I will wait a day or so for any additional
comments, and then send a v3.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/