Re: [xml] win32 thread safety patch

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

I went to bed, and my last thought was ... doh!

The thread wrapper is unneeded, and not useful in libxml, as the threads
are created outside libxml.

A hybrid approach is needed. If I have time I'll create it (rough sketch
follows).


===
----- Original Message -----
From: "Igor Zlatkovic" <igor stud fh-frankfurt de>
To: "Robert Collins" <robert collins itdomain com au>; <xml gnome org>
Sent: Monday, January 14, 2002 9:20 AM
Subject: Re: [xml] win32 thread safety patch


> > 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.
>
> No, xmlFreeGlobalState is used on Unix. It is precisely the destructor
> function which is specified in the call to pthread_key_create.

Ah, so this always used, except on _MSC_VER ?

> Hm... I am not sure if I understood this well. What would you wrap
under 1),
> the thread execution routine, or _beginthread function?

The thread execution routine. It's pointless to discuss now (it's not a
useful solution for libxml's scope).

> It seems that you assume libxml is the only library a program uses.
Imagine
> what would happen if a client app would use two libraries which would
both
> solve the threading question the way you proposed.

Simply imagine the client creating the threads anyway they want! :].

> Looking at the code you sent, either me or you slightly misunderstood
the
> basics of TSD. A TSD key is allocated per process, not per thread.
Libxml
> needs exactly one TSD key per process. The client application may
allocate
> additional keys if it needs to store per-thread data. All threads of a
> process can use the same key when setting and getting their TSD data.

Thats fine - I think we both understand that.

> Am I missing something?


Hmm, it's possible. What I read was this:
tlstate is created once (one TLS key).
Then _per thread that uses libxml_ (found when TlsGetValue (tlstate)
returns NULL) a new
xmlGlobalStateCleanupHelperParams is created, and passed the current
thread handle, and the value that is being inserted into the TLS
variable is cached in the xmlGlobalStateCleanupHelperParams struct. A
new thread is spawned that waits on the handle for the thread to exit,
and then initiates cleanup of the data pointed at by the memory pointer
(the TLS value is now inaccessible because the thread has terminated).

My objection is to the thread-per-thread overhead.

Now it's obvious (now I'm a little more awake) that a cached copy of the
pointer is needed, along with the thread HANDLE from syncronisation. But
a thread-per thread approach is very wasteful, only one cleanup thread
should be needed.

Rob