Re: [bug #43404] gl_locale_name_default() thread issues on OS X

[Daiki Ueno]

Pádraig Brady <address@hidden> writes:

> There are many consequences to having multiple threads,
> so it's worth avoiding/isolating that where possible.
>
> You mentioned it would be conditionalized on pthread_is_threaded_np
> for performance I suppose, as threaded progs would already
> have to deal with the consequences of a separate thread.
> So races in pthread_is_threaded_np would only be a small
> performance issue.
>
> Seems like a good idea to me.

Thanks for the comment.  Here is a tentative patch based on the
PostgreSQL patches.  Perhaps it might require a copyright assignment
from the original author (Cc'ed).

>From d8e9c96ef0c1e1c1c410cfbaec547c2e3d442dbe Mon Sep 17 00:00:00 2001
From: Daiki Ueno <address@hidden>
Date: Tue, 14 Oct 2014 12:47:06 +0900
Subject: [PATCH] localename: Avoid implicit thread creation in CoreFoundation

Problem reported by Peter Eisentraut at:
http://savannah.gnu.org/bugs/?43404.
Since some CoreFoundation functions (CFLocaleCopyCurrent for instance)
create a new thread internally, it would be better to move the calls
into a subprocess, when the caller is single-threaded.
Based on the patch created by Noah Misch:
http://www.postgresql.org/message-id/address@hidden
* tests/test-localename.c (test_locale_name_default) [__APPLE__]:
Check the return value of pthread_is_threaded_np before and after the
gl_locale_name_default.
* m4/localename.m4 (gl_LOCALENAME): Check pthread_is_threaded_np.
* lib/localename.c [__APPLE__]: Include <pthread.h>, <signal.h>, and
<unistd.h> if pthread_is_threaded_np is available.
(gl_locale_name_default_from_CoreFoundation) [__APPLE__]: New
function, split off from gl_locale_name_default.
(gl_locale_name_default_from_CoreFoundation_forked) [__APPLE__]: New
function.
(gl_locale_name_default) [__APPLE__]: Call _from_CoreFoundation or
_from_CoreFoundation_forked depending on whether or not the caller is
multi-threaded.
---
 lib/localename.c        | 161 ++++++++++++++++++++++++++++++++++++++++--------
 m4/localename.m4        |   2 +-
 tests/test-localename.c |  16 ++++-
 3 files changed, 151 insertions(+), 28 deletions(-)

diff --git a/lib/localename.c b/lib/localename.c
index 78dc344..745bc92 100644
--- a/lib/localename.c
+++ b/lib/localename.c
@@ -51,6 +51,12 @@
 # elif HAVE_CFPREFERENCESCOPYAPPVALUE
 #  include <CoreFoundation/CFPreferences.h>
 # endif
+# if HAVE_PTHREAD_IS_THREADED_NP
+#  define _DARWIN_C_SOURCE 1
+#  include <pthread.h>
+#  include <signal.h>
+#  include <unistd.h>
+# endif
 #endif
 
 #if defined _WIN32 || defined __WIN32__
@@ -2836,6 +2842,122 @@ gl_locale_name_environ (int category, const char 
*categoryname)
   return NULL;
 }
 
+#if HAVE_CFLOCALECOPYCURRENT || HAVE_CFPREFERENCESCOPYAPPVALUE
+static char *
+gl_locale_name_default_from_CoreFoundation (void)
+{
+  char namebuf[256];
+  char *localename = "C";
+# if HAVE_CFLOCALECOPYCURRENT /* Mac OS X 10.3 or newer */
+  CFLocaleRef locale = CFLocaleCopyCurrent ();
+  CFStringRef name = CFLocaleGetIdentifier (locale);
+
+  if (CFStringGetCString (name, namebuf, sizeof (namebuf),
+                         kCFStringEncodingASCII))
+    {
+      gl_locale_name_canonicalize (namebuf);
+      localename = namebuf;
+    }
+  CFRelease (locale);
+# elif HAVE_CFPREFERENCESCOPYAPPVALUE /* Mac OS X 10.2 or newer */
+  CFTypeRef value =
+    CFPreferencesCopyAppValue (CFSTR ("AppleLocale"),
+                              kCFPreferencesCurrentApplication);
+  if (value != NULL
+      && CFGetTypeID (value) == CFStringGetTypeID ()
+      && CFStringGetCString ((CFStringRef)value,
+                            namebuf, sizeof (namebuf),
+                            kCFStringEncodingASCII))
+    {
+      gl_locale_name_canonicalize (namebuf);
+      localename = namebuf;
+    }
+# endif
+  return strdup (localename);
+}
+
+# if HAVE_PTHREAD_IS_THREADED_NP
+static char *
+gl_locale_name_default_from_CoreFoundation_forked (void)
+{
+  char namebuf[256];
+  char *localename = "C";
+  int fd[2];
+  sigset_t sigs, old_sigs;
+  pid_t pid;
+  int child_status;
+  ssize_t nread;
+
+  /* Block SIGCHLD so we can get an exit status.  */
+  sigemptyset (&sigs);
+  sigaddset (&sigs, SIGCHLD);
+  sigprocmask (SIG_BLOCK, &sigs, &old_sigs);
+
+  if (pipe (fd) < 0)
+    goto done;
+
+  pid = fork ();
+  if (pid < 0)
+    {
+      close (fd[0]);
+      close (fd[1]);
+      goto done;
+    }
+
+  if (pid == 0)
+    {
+      char *locname = gl_locale_name_default_from_CoreFoundation ();
+      size_t locname_len = strlen (locname);
+      int status = EXIT_SUCCESS;
+
+      if (close (fd[0]) < 0)
+        status = EXIT_FAILURE;
+
+      if (write (fd[1], locname, locname_len) < locname_len)
+        status = EXIT_FAILURE;
+
+      if (close (fd[1]) < 0)
+        status = EXIT_FAILURE;
+
+      free (locname);
+      _exit (status);
+    }
+
+  if (close (fd[1]) < 0)
+    {
+      close (fd[0]);
+      goto done;
+    }
+
+  if (waitpid (pid, &child_status, 0) != pid
+      || !(WIFEXITED (child_status)
+          && WEXITSTATUS (child_status) == EXIT_SUCCESS))
+    {
+      close (fd[0]);
+      close (fd[1]);
+      goto done;
+    }
+
+  nread = read (fd[0], namebuf, sizeof (namebuf));
+  if (nread <= 0 || nread == sizeof (namebuf))
+    {
+      close (fd[0]);
+      goto done;
+    }
+
+  if (close (fd[0]) < 0)
+    goto done;
+
+  namebuf[nread] = '\0';
+  localename = namebuf;
+
+ done:
+  sigprocmask (SIG_SETMASK, &old_sigs, NULL);
+  return strdup (localename);
+}
+# endif
+#endif
+
 const char *
 gl_locale_name_default (void)
 {
@@ -2888,32 +3010,19 @@ gl_locale_name_default (void)
 
     if (cached_localename == NULL)
       {
-        char namebuf[256];
-#  if HAVE_CFLOCALECOPYCURRENT /* Mac OS X 10.3 or newer */
-        CFLocaleRef locale = CFLocaleCopyCurrent ();
-        CFStringRef name = CFLocaleGetIdentifier (locale);
-
-        if (CFStringGetCString (name, namebuf, sizeof (namebuf),
-                                kCFStringEncodingASCII))
-          {
-            gl_locale_name_canonicalize (namebuf);
-            cached_localename = strdup (namebuf);
-          }
-        CFRelease (locale);
-#  elif HAVE_CFPREFERENCESCOPYAPPVALUE /* Mac OS X 10.2 or newer */
-        CFTypeRef value =
-          CFPreferencesCopyAppValue (CFSTR ("AppleLocale"),
-                                     kCFPreferencesCurrentApplication);
-        if (value != NULL
-            && CFGetTypeID (value) == CFStringGetTypeID ()
-            && CFStringGetCString ((CFStringRef)value,
-                                   namebuf, sizeof (namebuf),
-                                   kCFStringEncodingASCII))
-          {
-            gl_locale_name_canonicalize (namebuf);
-            cached_localename = strdup (namebuf);
-          }
-#  endif
+        /* If the caller is a single-threaded process, isolate the
+           CoreFoundation calls, which may create a new thread, to a
+           separate process.
+          Based on the approach proposed by Noah Misch in:
+           http://www.postgresql.org/message-id/address@hidden
+       */
+        if (!pthread_is_threaded_np ())
+          cached_localename
+           = gl_locale_name_default_from_CoreFoundation_forked ();
+        else
+          cached_localename
+           = gl_locale_name_default_from_CoreFoundation ();
+
         if (cached_localename == NULL)
           cached_localename = "C";
       }
diff --git a/m4/localename.m4 b/m4/localename.m4
index d865c66..499a59e 100644
--- a/m4/localename.m4
+++ b/m4/localename.m4
@@ -8,5 +8,5 @@ AC_DEFUN([gl_LOCALENAME],
 [
   AC_REQUIRE([gt_LC_MESSAGES])
   AC_REQUIRE([gt_INTL_MACOSX])
-  AC_CHECK_FUNCS([setlocale uselocale])
+  AC_CHECK_FUNCS([setlocale uselocale pthread_is_threaded_np])
 ])
diff --git a/tests/test-localename.c b/tests/test-localename.c
index df6c1d6..1862f02 100644
--- a/tests/test-localename.c
+++ b/tests/test-localename.c
@@ -711,10 +711,24 @@ test_locale_name_environ (void)
 static void
 test_locale_name_default (void)
 {
-  const char *name = gl_locale_name_default ();
+  const char *name;
+  int is_threaded;
+
+  /* Check if the CoreFoundation calls are properly isolated and
+     don't create additional threads.  */
+#if (HAVE_CFLOCALECOPYCURRENT || HAVE_CFPREFERENCESCOPYAPPVALUE) \
+  && HAVE_PTHREAD_IS_THREADED_NP
+  is_threaded = pthread_is_threaded_np ();
+#endif
 
+  name = gl_locale_name_default ();
   ASSERT (name != NULL);
 
+#if (HAVE_CFLOCALECOPYCURRENT || HAVE_CFPREFERENCESCOPYAPPVALUE) \
+  && HAVE_PTHREAD_IS_THREADED_NP
+  ASSERT (pthread_is_threaded_np () == is_threaded);
+#endif
+
   /* Only Mac OS X and Windows have a facility for the user to set the default
      locale.  */
 #if !((defined __APPLE__ && defined __MACH__) || (defined _WIN32 || defined 
__WIN32__ || defined __CYGWIN__))
-- 
1.9.3

Regards,
--
Daiki Ueno