Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
Date: Jul 18, 2020, 03:09 UTC
Message-ID: <CAHd-oW6mvaANTBX5sVEi49s_Ku71-ZXqhyePtkwbCKaTK7tm4Q@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2007161214270.54@tvgsbejvaqbjf.bet>

Hi, Dscho

On Thu, Jul 16, 2020 at 9:56 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 11 quoted lines

>
> On Fri, 26 Jun 2020, Matheus Tavares wrote:
>
> > +     value = pthread_getspecific(hexbuf_array_key);
> > +     if (value) {
> > +             ha = (struct hexbuf_array *) value;
> > +     } else {
> > +             ha = xmalloc(sizeof(*ha));
>
> I just realized (while trying to debug something independent) that this
> leaves `ha->idx` uninitialized.

Thanks for catching that! I fixed it in my local branch.

Show 5 quoted lines

> But as I mentioned before, I would be much more in favor of abandoning
> this thread-local idea (because it is _still_ fragile, as the same thread
> could try to make use of more than four hex values in the same `printf()`,
> for example) and instead using Coccinelle to convert all those
> `oid_to_hex()` calls to `oid_to_hex_r()` calls.

Yeah, I agree that removing oid_to_hex() in favor of oid_to_hex_r() would be great. Unfortunately, I only used Coccinelle for basic things, such as function renaming. And I won't have the time to study it further at the moment :( Therefore, I think I'll ask Junio to drop this series for now, until I or someone else finds some time to work on the semantic patch.

Alternatively, if using thread-local storage is still an option, I think I might have solved the problems we had in the previous iteration with memory leaks on Windows. I changed our pthread_key_create() emulation to start using the destructor callback on Windows, through the Fiber Local Storage (FLS) API. As the documentation says [1] "If no fiber switching occurs, FLS acts exactly the same as thread local storage". The advantage over TLS is that FLSAlloc() does take a callback parameter.

I also removed the ugly `#ifdef HAVE_THREADS` guards on the last patch, as you suggested, and added some tests for our pthread_key emulation. In case you want to take a look to see if it might be worth pursuing this route, the patches are here: https://github.com/matheustavares/git/commits/safe_oid_to_hex_v3

[1]: https://docs.microsoft.com/en-us/windows/win32/procthread/fibers#fiber-local-storage