Re: [PATCH 28/28] attr: convert to new threadsafe API

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2016, 18:23 UTC
Message-ID: <xmqqa8eaagoq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kaQKDdJfMOjDKEK_dZJhgj+R7rByQS++B3OOBy6uO1x2w@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 31 quoted lines

>> I find this description a bit confusing.  At least the way I
>> envisioned was that when this piece of code is run by multiple
>> people at the same time,
>>
>>         static struct git_attr_check *check = NULL;
>>         git_attr_check_initl(&check, ...);
>>
>> we won't waste the "check" by allocated by the first one by
>> overwriting it with another one allocated by the second one.  So
>> "the same arguments" does not come into the picture.  A single
>> variable is either
>>
>>  * already allocated and initialised by the an earlier call to
>>    initl() by somebody else, or
>>
>>  * originally NULL when you call initl(), and the implementation
>>    makes sure that other people wait while you allocate, initialise
>>    and assign it to the variable, or
>>
>>  * originally NULL when you call initl(), but the implementation
>>    notices that somebody else is doing the second bullet point
>>    above, and you wait until that somebody else finishes and then
>>    you return without doing anything (because by that time, "check"
>>    is filled by that other party doing the second bullet point
>>    above).
>>
>> There is no need to check for "the same arguments".
>>
>
> I see. So we assume that there are no different arguments at the same time,
> i.e. all threads run the same code when it comes to attrs.

Sorry, but I fail to see how you can jump to that conclusion. Puzzled.

You can have many different callsites (think: hits "git grep" finds) that call git_attr_check_initl() and they all may be asking for different set of attributes. As long as they are using different "check" instance to receive these sets of attributes, they are OK.

It is insane to use the same "check" variable to receive sets of attributes for different attributes, be it from the same call or different one, it is insane to do this:

	func(char *anotherattr)
	{
		static struct git_attr_check *check = NULL;
		git_attr_check_initl(&check, "one", anotherattr, ...);

		... use "check" to ask question ...
	}

The whole point of having a static, initialize-once instance of "check" is so that initl() can do potentially heavy preparation just once and keep reusing it. Allowing a later caller of func() to pass a value of anotherattr that is different from the one used in the first call that did cause initl() to allocate-initialise-assign to "check" is simply insane, even there is no threading issue.

And in a threaded environment it is even worse; the first thread may call initl() to get one set of attributes in "check" and it may be about to ask the question, while the second call may call initl() and by your definition it will notice they have different sets of attributes and returns different "check"? Either the earlier one is leaked, or it gets destroyed even though the first thread hasn't finished with "check" it got.

It is perfectly OK to drop "static" from the above example code. Then it no longer is insane--it is perfectly normal code whose inefficiency cannot be avoided because it wants to do dynamic queries.