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

[Matheus Tavares Bernardino <matheus.bernardino@usp.br>]

Hi, Dscho

On Thu, Jul 16, 2020 at 9:56 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> 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.

> 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