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

Stefan Beller [off-list ref] writes:

This revamps the API of the attr subsystem to be thread safe.
Before we had the question and its results in one struct type.
The typical usage of the API was

    static struct git_attr_check *check;

    if (!check)
        check = git_attr_check_initl("text", NULL);

    git_check_attr(path, check);
    act_on(check->value[0]);

This has a couple of issues when it comes to thread safety:

* the initialization is racy in this implementation. To make it
  thread safe, we need to acquire a mutex, such that only one
  thread is executing the code in git_attr_check_initl.
  As we do not want to introduce a mutex at each call site,
  this is best done in the attr code. However to do so, we need
  to have access to the `check` variable, i.e. the API has to
  look like
    git_attr_check_initl(struct git_attr_check*, ...);

Make that a double-asterisk.  The same problem appears in an updated
example in technical/api-gitattributes.txt doc, but the example in
the commit log message (below) is correct.

The usage of the new API will be:

    /*
     * The initl call will thread-safely check whether the
     * struct git_attr_check has been initialized. We only
     * want to do the initialization work once, hence we do
     * that work inside a thread safe environment.
     */
    static struct git_attr_check *check;
    git_attr_check_initl(&check, "text", NULL);

    /*
     * Obtain a pointer to a correctly sized result
     * statically allocated on the stack; this macro:
     */
    GIT_ATTR_RESULT_INIT_FOR(myresult, 1);

Are you sure about this?  We've called attr_check_initl() already so
if this is declaring myresult, it would be decl-after-stmt.

    /* Perform the check and act on it: */
    git_check_attr(path, check, myresult);
    act_on(myresult->value[0]);

    /*
     * No need to free the check as it is static, hence doesn't leak
     * memory. The result is also static, so no need to free there either.
     */

The latter half is questionable.  If it is "static" it wouldn't be
thread safe, no?  I think the diff in this patch for archive.c shows
that we only expect

	struct git_attr_result result[2];

upfront without RESULT_INIT_FOR(), and the reason why there is no
need to free the result[] is because it is on the stack.  And each
element in result[] may point at a string, but the string belongs to
the attr subsystem and must not be freed.

On Mon, Oct 24, 2016 at 11:55 AM, Junio C Hamano [off-list ref] wrote:

Make that a double-asterisk.  The same problem appears in an updated
example in technical/api-gitattributes.txt doc, but the example in
the commit log message (below) is correct.

The implementation is actually using a double pointer, see below,
I forgot commit message and documentation

quoted

    GIT_ATTR_RESULT_INIT_FOR(myresult, 1);

Are you sure about this?  We've called attr_check_initl() already so
if this is declaring myresult, it would be decl-after-stmt.

I forgot to update the commit message and Documentation.
GIT_ATTR_RESULT_INIT_FOR is gone in the header
and in the implementation.  I'll update that patch
to be consistent throughout all of {Documentation,
commit message, implementation}.

The latter half is questionable.  If it is "static" it wouldn't be
thread safe, no?  I think the diff in this patch for archive.c shows
that we only expect

        struct git_attr_result result[2];

upfront without RESULT_INIT_FOR(), and the reason why there is no
need to free the result[] is because it is on the stack.  And each
element in result[] may point at a string, but the string belongs to
the attr subsystem and must not be freed.

Same as above, it's bogus.

Thanks,
Stefan

On Tue, Oct 25, 2016 at 2:18 AM, Stefan Beller [off-list ref] wrote:

On Mon, Oct 24, 2016 at 11:55 AM, Junio C Hamano [off-list ref] wrote:

quoted

Make that a double-asterisk.  The same problem appears in an updated
example in technical/api-gitattributes.txt doc, but the example in
the commit log message (below) is correct.

The implementation is actually using a double pointer, see below,
I forgot commit message and documentation

quoted

quoted

    GIT_ATTR_RESULT_INIT_FOR(myresult, 1);

Are you sure about this?  We've called attr_check_initl() already so
if this is declaring myresult, it would be decl-after-stmt.

I forgot to update the commit message and Documentation.
GIT_ATTR_RESULT_INIT_FOR is gone

I was asking whether this function/macro was not thread-safe and found
out it didn't exist as well, and it's bed time so I'm stopping. Will
continue my skimming on the next re-roll :)
-- 
Duy