Re: [xml] [patch] make libxml2 slightly more thread-friendly

[Ted Phelps <phelps mantara com>]

Hi Daniel,

As always, thank you for your speedy reply.

Daniel Veillard writes:
> Okay, fixing the problem sounds fine of course, but I have a couple of 
> issues with the patch:
>
>    - all thread implementation specific code in libxml2 is contained in
>      threads.c module, that patch breal that containment, I would really
>      prefer if this code was kept in threads.c

I thought you might.  I initially chose not to because I didn't want to
add new functions to the shared object, but I'm happy to do so if you
are.  Or do you have some mechanism to prevent functions from being
exported to the aplication?


>    - it looks to me that this code would break if libxml2 is configured
>      without thread support (configure --without-threads)

Oops.  You are correct.  That should now be fixed.

>    - it uses the keyword 'volatile' which is not used anywhere else in 
>      libxml2, I would prefer to avoid it if possible to maximize portability
>      (plus its semantic is IMHO rather fuzzy)

I included the volatile keyword for the Windows code because the
declaration of InterlockedCompareExchangePointer indicates that the
first parameter should be of type 'PVOID volatile *'.  I believe that we
can safely leave this out and I have removed it from the patch below.

For the BeOS implementation, I have changed 'volatile' to vint32,
following the definition of atomic_add:

    http://www.beunited.org/bebook/The%20Support%20Kit/Functions.html

You may want to consider changing run_once_init to be a vint32 too.


> I'm not sure how to best achieve those improvements, as this code
> really mix the xmlInitParser with really thread-specific code.

I have shifted the thread-related code into threads.c, leaving only
calls to lock and unlock a global mutex.

> Also we had trouble in the past with the xmlInitGlobals()/
> xmlInitThreads()/xmlInitMemory() ordering of initializers, the
> interraction are relatively complex to debug I hope the new code
> doesn't have side effect, that seems true as it uses direct memory
> allocations, it's just something to keep in mind.

I believe this patch should be immmune to such interactions.

> Ted, do you think you could try to rewrite the patch to address the 3
> issues listed before, this may require separing the xmlInitParser into
> one routine when thread are not compiled in, and one in threads.c when
> thread support is compiled in (but that's just a suggestion).

No worries.  Here's a second attempt.  Please have a look and let me
know if you'd like me to try a different approach or make additional
changes.

Thanks again,
-Ted


diff -Naurp libxml2-2.6.27.orig/include/libxml/threads.h libxml2-2.6.27/include/libxml/threads.h
--- libxml2-2.6.27.orig/include/libxml/threads.h        2005-09-04 21:57:21.000000000 +0100
+++ libxml2-2.6.27/include/libxml/threads.h     2007-02-12 15:26:12.000000000 +0000
@@ -54,6 +54,11 @@ XMLPUBFUN void XMLCALL                       
 XMLPUBFUN void XMLCALL                 
                        xmlFreeRMutex   (xmlRMutexPtr tok);
 
+XMLPUBFUN void XMLCALL
+                       xmlGlobalInitMutexLock(void);
+XMLPUBFUN void XMLCALL
+                       xmlGlobalInitMutexUnlock(void);
+
 /*
  * Library wide APIs.
  */
diff -Naurp libxml2-2.6.27.orig/parser.c libxml2-2.6.27/parser.c
--- libxml2-2.6.27.orig/parser.c        2006-10-17 15:28:27.000000000 +0100
+++ libxml2-2.6.27/parser.c     2007-02-12 15:41:38.000000000 +0000
@@ -12839,26 +12839,34 @@ xmlInitParser(void) {
     if (xmlParserInitialized != 0)
        return;
 
-    if ((xmlGenericError == xmlGenericErrorDefaultFunc) ||
-       (xmlGenericError == NULL))
-       initGenericErrorDefaultFunc(NULL);
-    xmlInitGlobals();
-    xmlInitThreads();
-    xmlInitMemory();
-    xmlInitCharEncodingHandlers();
-    xmlDefaultSAXHandlerInit();
-    xmlRegisterDefaultInputCallbacks();
+#ifdef LIBXML_THREAD_ENABLED
+    xmlGlobalInitMutexLock();
+    if (xmlParserInitialized == 0) {
+#endif
+       if ((xmlGenericError == xmlGenericErrorDefaultFunc) ||
+           (xmlGenericError == NULL))
+           initGenericErrorDefaultFunc(NULL);
+       xmlInitGlobals();
+       xmlInitThreads();
+       xmlInitMemory();
+       xmlInitCharEncodingHandlers();
+       xmlDefaultSAXHandlerInit();
+       xmlRegisterDefaultInputCallbacks();
 #ifdef LIBXML_OUTPUT_ENABLED
-    xmlRegisterDefaultOutputCallbacks();
+       xmlRegisterDefaultOutputCallbacks();
 #endif /* LIBXML_OUTPUT_ENABLED */
 #ifdef LIBXML_HTML_ENABLED
-    htmlInitAutoClose();
-    htmlDefaultSAXHandlerInit();
+       htmlInitAutoClose();
+       htmlDefaultSAXHandlerInit();
 #endif
 #ifdef LIBXML_XPATH_ENABLED
-    xmlXPathInit();
+       xmlXPathInit();
+#endif
+       xmlParserInitialized = 1;
+#ifdef LIBXML_THREAD_ENABLED
+    }
+    xmlGlobalInitMutexUnlock();
 #endif
-    xmlParserInitialized = 1;
 }
 
 /**
diff -Naurp libxml2-2.6.27.orig/threads.c libxml2-2.6.27/threads.c
--- libxml2-2.6.27.orig/threads.c       2006-06-26 13:16:03.000000000 +0100
+++ libxml2-2.6.27/threads.c    2007-02-12 16:02:49.000000000 +0000
@@ -139,6 +139,7 @@ struct _xmlRMutex {
 static pthread_key_t   globalkey;
 static pthread_t       mainthread;
 static pthread_once_t once_control = PTHREAD_ONCE_INIT;
+static pthrad_mutex_t global_init_lock = PTHREAD_MUTEX_INITIALIZER;
 #elif defined HAVE_WIN32_THREADS
 #if defined(HAVE_COMPILER_TLS)
 static __declspec(thread) xmlGlobalState tlstate;
@@ -152,11 +153,14 @@ static struct
     DWORD done;
     DWORD control;
 } run_once = { 0, 0 };
+static LPCRITICAL_SECTION global_init_lock = NULL;
 /* endif HAVE_WIN32_THREADS */
 #elif defined HAVE_BEOS_THREADS
 int32 globalkey = 0;
 thread_id mainthread = 0;
 int32 run_once_init = 0;
+static int32 global_init_lock = -1;
+static int32 global_init_count = 0;
 #endif
 
 static xmlRMutexPtr    xmlLibraryLock = NULL;
@@ -413,6 +417,85 @@ xmlRMutexUnlock(xmlRMutexPtr tok ATTRIBU
 #endif
 }
 
+/**
+ * xmlGlobalInitMutexLock
+ *
+ * Makes sure that the global initialization mutex is initialized and
+ * locks it.
+ */
+void
+xmlGlobalInitMutexLock(void)
+{
+    /* Make sure the global init lock is initialized and then lock it. */
+#ifdef HAVE_PTHREAD_H
+    int err;
+
+    /* The mutex is statically initialized, so we just lock it. */
+    pthread_mutex_lock(&global_init_lock);
+#elif defined HAVE_WIN32_THREADS
+    LPCRITICAL_SECTION cs;
+
+    /* Create a new critical section */
+    if (global_init_lock == NULL) {
+       cs = malloc(sizeof(CRITICAL_SECTION));
+       InitializeCriticalSection(cs);
+
+       /* Swap it into the global_init_lock */
+       InterlockedCompareExchangePointer(&global_init_lock, cs, NULL);
+
+       /* If another thread successfully recorded its critical
+        * section in the global_init_lock then discard the one
+        * allocated by this thread. */
+       if (global_init_lock != cs) {
+           free(cs);
+       }
+    }
+
+    /* Lock the chosen critical section */
+    EnterCriticalSection(global_init_lock);
+#elif defined HAVE_BEOS_THREADS
+    int32 sem;
+
+    /* Allocate a new semaphore */
+    sem = create_sem(1, "xmlGlobalinitMutex");
+
+    while (global_init_lock == -1) {
+       if (atomic_add(&global_init_count, 1) == 0) {
+           global_init_lock = sem;
+       } else {
+           snooze(1);
+           atomic_add(&global_init_count, -1);
+       }
+    }
+
+    /* If another thread successfully recorded its critical
+     * section in the global_init_lock then discard the one
+     * allocated by this thread. */
+    if (global_init_lock != sem)
+       delete_sem(sem);
+
+    /* Acquire the chosen semaphore */
+    if (acquire_sem(global_init_lock) != B_NO_ERROR) {
+#ifdef DEBUG_THREADS
+       xmlGenericError(xmlGenericErrorContext, "xmlGlobalInitMutexLock():BeOS:Couldn't acquire semaphore\n");
+       exit();
+#endif
+    }
+#endif
+}
+
+void
+xmlGlobalInitMutexUnlock(void)
+{
+#ifdef HAVE_PTHREAD_H
+    pthread_mutex_unlock(&global_init_lock);
+#elif defined HAVE_WIN32_THREADS
+    LeaveCriticalSection(global_init_lock);
+#elif defined HAVE_BEOS_THREADS
+    release_sem(global_init_lock);
+#endif
+}
+
 /************************************************************************
  *                                                                     *
  *                     Per thread global state handling                *