🎉 Celebrating 25 Years of GameDev.net! 🎉

Not many can claim 25 years on the Internet! Join us in celebrating this milestone. Learn more about our history, and thank you for being a part of our community!

scriptstdstring setlocale multithread issue (crashes) and proposed fix

Started by
2 comments, last by gjl 2 days, 22 hours ago

Hi,

I have noticed that the string to float conversion make multithreaded programs crash when used intensively in multiple threads in apps that are using non-C locale. This is due to the fact that setlocale is not thread safe at all.

Please find below a proposed patch to overcome this issue (using uselocal instead, with a custom implementation for windows as it does not exist there):

Index: scriptstdstring/scriptstdstring.cpp
===================================================================
--- scriptstdstring/scriptstdstring.cpp	(original)
+++ scriptstdstring/scriptstdstring.cpp	(working copy)
@@ -6,7 +6,145 @@
 #include <stdlib.h> // strtod()
 #ifndef __psp2__
 	#include <locale.h> // setlocale()
+#ifdef _WIN32
+// win32 does not have the thread safe uselocal feature
+// here is an implementation
+struct LocaleStruct
+{
+	char* localeStrings[LC_MAX + 1];
+};
+typedef LocaleStruct* locale_t;
+
+#define LC_GLOBAL_LOCALE ((locale_t)-1)
+#define LC_COLLATE_MASK  (1<<0)
+#define LC_CTYPE_MASK    (1<<1)
+#define LC_MONETARY_MASK (1<<2)
+#define LC_NUMERIC_MASK  (1<<3)
+#define LC_TIME_MASK     (1<<4)
+#define LC_MESSAGES_MASK (1<<5)
+#define LC_ALL_MASK      (LC_COLLATE_MASK | LC_CTYPE_MASK | LC_MESSAGES_MASK | \
+			              LC_MONETARY_MASK | LC_NUMERIC_MASK | LC_TIME_MASK)
+
+
+inline void freelocale(locale_t locale)
+{
+	if (locale != NULL && locale != LC_GLOBAL_LOCALE)
+	{
+		for (int i = LC_MIN; i <= LC_MAX; i++)
+		{
+			if (locale->localeStrings[i] != NULL)
+				free(locale->localeStrings[i]);
+		}
+		free(locale);
+	}
+}
+
+inline locale_t duplocale(locale_t base)
+{
+	locale_t loc = (locale_t)malloc(sizeof(LocaleStruct));
+	if (loc)
+	{
+		// use the equivalent of 
+		if (base == LC_GLOBAL_LOCALE)
+			base = NULL;
+	}
+}
+
+inline locale_t newlocale(int categoryMask, const char* locale, locale_t base)
+{
+	// allocate new object
+	locale_t loc = (locale_t)malloc(sizeof(LocaleStruct));
+	if (loc)
+	{
+		// duplicate locale under lock
+		_lock_locales();
+		for (int i = LC_MIN; i <= LC_MAX; i++)
+		{
+			// either copy existing or use new depending on mask
+			char const* localeToSet = locale;
+			if ((i & categoryMask) == 0)
+			{
+				if (base == NULL || base == LC_GLOBAL_LOCALE)
+					localeToSet = setlocale(i, NULL);
+				else
+					localeToSet = base->localeStrings[i];
+			}
+
+			// copy locale string
+			if (localeToSet)
+				loc->localeStrings[i] = strdup(locale);
+			else
+				loc->localeStrings[i] = NULL;
+		}
+		_unlock_locales();
+	}
+	// free old object
+	freelocale(base);
+	return loc;
+}
+
+inline locale_t uselocale(locale_t new_locale)
+{
+	// per-thread locale data cache
+	__declspec(thread) static
+		struct OwnedLocalePtr
+	{
+		OwnedLocalePtr(locale_t locale) :
+			localePtr(locale) {}
+		~OwnedLocalePtr()
+		{
+			freelocale(localePtr);
+		}
+		locale_t localePtr;
+	} staticLocaleData(NULL);
+
+	locale_t old_locale = LC_GLOBAL_LOCALE;
+
+	// check if current locale is per thread
+	const bool isPerThread = (_configthreadlocale(0) == _ENABLE_PER_THREAD_LOCALE);
+
+	// Retrieve the current thread-specific locale
+	if (isPerThread)
+	{
+		if (staticLocaleData.localePtr == NULL)
+		{
+			// create new data as it has not been initialized yet
+			staticLocaleData.localePtr = newlocale(LC_ALL_MASK, NULL, NULL);
+		}
+
+		// store pointer to be returned
+		old_locale = staticLocaleData.localePtr;
+	}
+
+	if (new_locale == LC_GLOBAL_LOCALE)
+	{
+		// Restore the global locale
+		_configthreadlocale(_DISABLE_PER_THREAD_LOCALE);
+	}
+	else if (new_locale != NULL)
+	{
+		// Configure the thread to set the locale only for this thread
+		_configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
+
+		// Set new locale categories
+		for (int i = LC_MIN; i <= LC_MAX; i++)
+		{
+			if (new_locale->localeStrings[i] != NULL)
+				setlocale(i, new_locale->localeStrings[i]);
+		}
+	}
+
+	// store in local thread storage if not NULL
+	if (new_locale != NULL)
+		staticLocaleData.localePtr = new_locale;
+	return old_locale;
+}
+
+#else
+// thread safe uselocal functions
+#include <xlocale.h>
 #endif
+#endif
 
 using namespace std;
 
@@ -837,9 +975,9 @@
 	// locale is ",".
 #if !defined(_WIN32_WCE) && !defined(ANDROID) && !defined(__psp2__)
 	// Set the locale to C so that we are guaranteed to parse the float value correctly
-	char *tmp = setlocale(LC_NUMERIC, 0);
-	string orig = tmp ? tmp : "C";
-	setlocale(LC_NUMERIC, "C");
+	// using the thread safe uselocal instead of setlocale
+	locale_t locale = newlocale(LC_NUMERIC_MASK, "C", NULL);
+	locale_t orig_locale = uselocale(locale);
 #endif
 
 	double res = strtod(val.c_str(), &end);
@@ -846,7 +984,8 @@
 
 #if !defined(_WIN32_WCE) && !defined(ANDROID) && !defined(__psp2__)
 	// Restore the locale
-	setlocale(LC_NUMERIC, orig.c_str());
+	uselocale(orig_locale);
+	freelocale(locale);
 #endif
 
 	if( byteCount )

It would be nice if this (or any other fix - you could make it shorter if not fully re-implementing uselocal on windows) could be incorporated into the mainline. Thanks again for your great work!

Advertisement

Thanks for letting know about this problem and providing a potential fix. I'll look into this.

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

BTW I have noticed that you already have a native re-implementation of strtod with asStringScanDouble, as far as I understand it:


double asStringScanDouble(const char *string, size_t *numScanned)
{
	// I decided to do my own implementation of strtod() because this function
	// doesn't seem to be present on all systems. iOS 5 for example doesn't appear 
	// to include the function in the standard lib.
	
	// Another reason is that the standard implementation of strtod() is dependent
	// on the locale on some systems, i.e. it may use comma instead of dot for 
	// the decimal indicator. This can be avoided by forcing the locale to "C" with
	// setlocale(), but this is another thing that is highly platform dependent.

So if it is strictly equivalent, it is probably better and simpler to use it instead of the proposed fix above.

Advertisement