Re: [Libreoffice] Interesting difference in thread-specific data function semantics for Windows vs. Unix

On Sun, 2011-06-19 at 14:10 -0600, Tor Lillqvist wrote:
> On Windows, if the value of a key is changed with
>  osl_setThreadKeyData() (or its wrapper, ThreadData::setData()), the
>  destructor callback is called for the old value. (See the code in
>  sal/osl/w32/thread.c. On Unix this does not seem to happen. There
>  osl_setThreadKeyData() is just a thin wrapper for
>  pthread_setspecific(), and the documentation for that does not say
>  anything about the destructor being called. (Experimentation confirms
>  this, at least on Linux.)

Yeah, the documentation for pthread_key_create explicitly says...

"If the  value  associated with a key needs to be updated during the
lifetime of the thread, it may be necessary to release the storage
associated with the old value before the new  value is bound. Although
the pthread_setspecific() function could do this automatically, this
feature is not needed often enough to justify the added complexity.
Instead, the programmer is responsible for freeing the stale storage"

catch is that afaics you can't get the destructor originally set with
pthread_key_create back out from the api anywhere, so you'd need to pass
it around yourself.

>  So, the interesting question now then is whether this difference in 
>  osl_setThreadKeyData() semantics is intentional and known, and handled
>  specifically in those places in the code where thread-specific data is
>  used?

I'd say fat chance that it is intentional and handled correctly
anywhere.

>  Or whether the code assumes the semantics on Windows and just
>  then leaks on Unix? Or what...

Yeah, that'd be my guess.

So we could.

a) forget about it, and have the two platforms different (yuck)
b) make both the same as the pthreads one
c) make both the same as the windows one

If we want c), which is a more attractive and obvious api, then the
attached would do it I think. It does mean a change of the typedef of
oslThreadKey from sal_uInt32 to void*, which changes its size on 64bit.
So technically this is an incompatible change for 64bit binary
extensions that use this obscure api.

Given that the windows api returns a pointer here anyway, I guess this
is required for windows64 anyway, right ? And its sufficiently edge-case
that I doubt there's a single 64bit x86_64 binary linux extension out
there using this api.

C.

diff --git a/sal/inc/osl/thread.h b/sal/inc/osl/thread.h
index 4cd058b..9e088e6 100644
--- a/sal/inc/osl/thread.h
+++ b/sal/inc/osl/thread.h
@@ -68,7 +68,7 @@ typedef enum
 
 typedef sal_uInt32 oslThreadIdentifier;
 
-typedef sal_uInt32 oslThreadKey;
+typedef void* oslThreadKey;
 
 /** Create the thread, using the function-ptr pWorker as
     its main (worker) function. This functions receives in
diff --git a/sal/osl/unx/thread.c b/sal/osl/unx/thread.c
index 06730ac..b11edb0 100644
--- a/sal/osl/unx/thread.c
+++ b/sal/osl/unx/thread.c
@@ -35,6 +35,7 @@
 #include <osl/thread.h>
 #include <osl/nlsupport.h>
 #include <rtl/textenc.h>
+#include <rtl/alloc.h>
 #include <sal/macros.h>
 
 #if defined LINUX
@@ -975,17 +976,31 @@ oslThreadPriority SAL_CALL osl_getThreadPriority(const oslThread Thread)
     return Priority;
 }
 
+typedef struct _wrapper_pthread_key
+{
+    pthread_key_t m_key;
+    oslThreadKeyCallbackFunction pfnCallback;
+} wrapper_pthread_key;
+
 /*****************************************************************************/
 /* osl_createThreadKey */
 /*****************************************************************************/
 oslThreadKey SAL_CALL osl_createThreadKey( oslThreadKeyCallbackFunction pCallback )
 {
-    pthread_key_t key;
+    wrapper_pthread_key *pKey = 
(wrapper_pthread_key*)rtl_allocateMemory(sizeof(wrapper_pthread_key));
+
+    if (pKey)
+    {
+        pKey->pfnCallback = pCallback;
 
-    if (pthread_key_create(&key, pCallback) != 0)
-        key = 0;
+        if (pthread_key_create(&(pKey->m_key), pKey->pfnCallback) != 0)
+        {
+            rtl_freeMemory(pKey);
+            pKey = 0;
+        }
+    }
 
-    return ((oslThreadKey)key);
+    return ((oslThreadKey)pKey);
 }
 
 /*****************************************************************************/
@@ -993,7 +1008,12 @@ oslThreadKey SAL_CALL osl_createThreadKey( oslThreadKeyCallbackFunction 
pCallbac
 /*****************************************************************************/
 void SAL_CALL osl_destroyThreadKey(oslThreadKey Key)
 {
-    pthread_key_delete((pthread_key_t)Key);
+    wrapper_pthread_key *pKey = (wrapper_pthread_key*)Key;
+    if (pKey)
+    {
+        pthread_key_delete(pKey->m_key);
+        rtl_freeMemory(pKey);
+    }
 }
 
 /*****************************************************************************/
@@ -1001,7 +1021,8 @@ void SAL_CALL osl_destroyThreadKey(oslThreadKey Key)
 /*****************************************************************************/
 void* SAL_CALL osl_getThreadKeyData(oslThreadKey Key)
 {
-    return (pthread_getspecific((pthread_key_t)Key));
+    wrapper_pthread_key *pKey = (wrapper_pthread_key*)Key;
+    return pKey ? pthread_getspecific(pKey->m_key) : NULL;
 }
 
 /*****************************************************************************/
@@ -1009,7 +1030,21 @@ void* SAL_CALL osl_getThreadKeyData(oslThreadKey Key)
 /*****************************************************************************/
 sal_Bool SAL_CALL osl_setThreadKeyData(oslThreadKey Key, void *pData)
 {
-    return (pthread_setspecific((pthread_key_t)Key, pData) == 0);
+    sal_Bool bRet;
+    void *pOldData = NULL;
+    wrapper_pthread_key *pKey = (wrapper_pthread_key*)Key;
+    if (!pKey)
+        return sal_False;
+
+    if (pKey->pfnCallback)
+        pOldData = pthread_getspecific(pKey->m_key);
+
+    bRet = (pthread_setspecific(pKey->m_key, pData) == 0);
+
+    if (bRet && pKey->pfnCallback && pOldData)
+        pKey->pfnCallback(pOldData);
+
+    return bRet;
 }
 
 /*****************************************************************************/
diff --git a/sal/qa/osl/process/osl_Thread.cxx b/sal/qa/osl/process/osl_Thread.cxx
index aa6ad67..3cf8837 100644
--- a/sal/qa/osl/process/osl_Thread.cxx
+++ b/sal/qa/osl/process/osl_Thread.cxx
@@ -2099,9 +2104,6 @@ namespace osl_ThreadData
                     "ThreadData setData: ",
                     cData1 == 'a' && cData2 == 'b' && aChar == 'o'
                     );
-
-                delete [] pc2;
-                delete [] pc;
             }
 
         CPPUNIT_TEST_SUITE(setData);
@@ -2149,8 +2152,6 @@ namespace osl_ThreadData
                     "ThreadData setData: ",
                     cData1 == 'c' && cData2 == 'd' && aChar == 'i'
                     );
-
-                delete [] pc;
             }
 
         // setData then change the value in the address data pointer points,