Re: [xml] win32 thread safety patch

["Robert Collins" <robert collins itdomain com au>]

Ok. The patch looks fine to me - (I was checking for cygwin
interactions). It's all #ifdef'd safely, except for the

static void
xmlFreeGlobalState(void *state)

and related bits that are only protected with


#ifdef LIBXML_THREAD_ENABLED
#ifndef _MSC_VER

Which will break (AFAICT) on unix builds - that protection should
include the HAVE_WIN32_THREADS test.

However some error tests are missing: for example TlsAlloc may not
succeed, but it's not tested for failure, which could lead to serious
problems!

Se below for an alternative to the thread-per-TLS instance approach to
cleanup of per thread data.


===
----- Original Message -----
From: "Igor Zlatkovic" <igor stud fh-frankfurt de>

> a) This is not any simpler than what Serguei did, but is a solution. I
would
> welcome a patch.

It may not be simpler but it is more efficient :]. The overhead of an
extra thread per TLS value per thread is potentially large. I don't know
how many TLS values libXML uses, but if it used say 5, and a user app
accessed libxml via 5 threads we'd end up with 25 idle threads!

> b) Well, when you take a look at the code of the pthreads-win32
library
> (http://sources.redhat.com/pthreads-win32/), the pthread_key_create
> functionality which enables a destructor function to be called is
everything
> but trivial.
>
> Taking the required functionality from pthreads-win32 library would
mean
> taking a good part of it. More, we would then have one libxml2 binary
for
> the client code which uses C++ exception handling, one for the client
code
> which uses Windows structured exception handling, one for the code
which
> uses none of these but plays setjmp/longjmp, a myriad of binaries for
a
> myriad of possibilities other compilers might offer... Wouldn't it be
a lot
> better to use pthread interface in libxml2 and then link to the actual
> pthreads-win32 library? An additional library it is, still I would
welcome
> it much warmer than repeating two thirds of it's functionality in
libxml2.

Yup, that was my point in asking if anyone had tried using it :]. Not
being interested in a flamewar, and being quite happy with cygwin's
pthread functionality, I'm not going to push though!

> If there is a way to implement pthread_key_* functionality trivially,
so
> that any client code can link to the same binary and work, I would, of
> course, love to see it.

Well there is. It's not _trivial_  but it is pretty simple. Firstly the
library handling for TLS and TLS cleanup is orthogonal to that of the
client. As long as both end up being implemented by win32 calls, they
will interoperate.

Secondly the pthread_key functionality requires (in libxml only) 3
things.
1) A thread wrapper function. This function is called instead of the
thread to be started - so that returning from the thread function
invokes the cleanup code.
2) An ExitThread replacement macro (to prevent ExitThread() being called
which would shortcircuit the cleanup code.
3) The pthread_key_create, pthread_getspecific and pthread_setspecific
functions and the cleanupcode.

1) is trivial (it's just a function with the same parameters as
_beginthread). All it does is call the thread function with it's
parameter, then call the cleanup code (which can be embedded) and then
returns.
2) Is even more trivial.
3) Takes a little more care...

/* put these in a container of some sort. A list/array etc will do.
   It's out of scope of this example code - and libXML probably has
   a toolkit for that sort of thing anyway.
 */
struct _pthread_key
{
  DWORD dwTlsIndex;
  void (*theDestructor) (void *);
};

int
pthread_key_create (pthread_key_t *key, void (*destructor) (void *))
{
  struct _pthread_key aKey = calloc (sizeof (struct _pthread_key), 1);
  aKey->dwTlsIndex = TlsAlloc ();
  /* Check for errors (Missing from current patch BTW */
  aKey->theDestructor = destructor;
  *key = /* index returned from container */;
}

void *
pthread_getspecific (pthread_key_t key)
{
  struct _pthread_key *theKey = /* get struct from container */(key);
  return (void *) TlsGetValue (theKey->dwTlsIndex);
}

int
pthread_setspecific (pthread_key_t key, const void *value)
{
  struct _pthread_key *theKey = /* get struct from container */(key);
  if (!theKey)
    return EINVAL;
  /* Always succeeds according to MSDN it dwTlsIndex is valid */
  TlsSetValue (theKey->dwTlsIndex, value);
  return 0;
}

/* skipping pthread_key_delete as it's not used in libxml - if
implemented it should just remove the key from the container -
applications have to perform any syncronisation */

void
cleanup_code (void)
{
  for (/* each struct _pthread_key theKey in the container */)
  {
    void *theValue;
    if ((theValue = (void *) TlsGetValue (theKey->dwTlsIndex)))
    {
      theKey->theDestructor (value);
    }
  }
}


voila! one compact implementation of the pthread_key_create and related
functions.
Key notes: POSIX specifies that applications need to perform their own
syncronisation to ensure that use of pthread_key_create is threadsafe -
but the underlying implementation should be robust if they don't. So the
container above should be threadsafe - ie mutex  protected.

Some efficiencies can be made: If pthread_key is typecast to be a
(struct _pthread_key *) then with the addition of a magic number the
container lookups can be skipped. That would have obfuscated the code
here a little though,  which is why I skipped it.

Cheers,
Rob