lib, mgr: use per-thread locale on Linux

[JuhaSointusalo]

On Linux, Manager sometimes prints timestamps in C locale instead of the
locale user has chosen. This happens when Manager is formatting a
timestamp and at the same time a GUI RPC is in progress. GUI RPCs
temporarily set the global locale to C locale with SET_LOCALE.

Fix this by making SET_LOCALE per-thread locale aware on Linux.
SET_LOCALE's per-thread locale version has an assumption that it is used
only when the calling thread is already set to use C locale. This
assumption is not documented and not followed in Manager. Change the
code to actually set per-thread locale.

Set per-thread locale in Manager's RPC thread to cover for any code not
using SET_LOCALE.

Add and fix configure checks that check for per-thread locale support. Clean
up code and build system.

Fixes #2399.

[JuhaSointusalo]

@MarkJ62 Please test: https://drive.google.com/file/d/1N3ygPLzxgmZoRvXFzAhKgG02HNLakY4u/view?usp=sharing

@CharlieFenton I removed OS X < 10.4 code because you don't support it. Right? Please check I didn't break the Mac build too much.

[CharlieFenton]

These changes look good to me except for the following:

Are we still supporting the HAIKU OS? If so, does removing the following code from gui_rpc_client.h break that support?

#if defined(__HAIKU__)
#define NO_PER_THREAD_LOCALE 1
#endif

Note that the code in the original gui_rpc_client.h lines 811-834 make SET_LOCALE() and ~SET_LOCALE() empty functions which do nothing if uselocale() is available, so they are empty functions on all Macs we currently support. Likewise, lines 838-844 define them as empty functions on MSW.

This is because, when we have per-thread locales, AsyncRPC.cpp lines 143-144 and 153-154 permanently set the locale for all RPC threads for MSW and Mac, respectively. So the only cases where SET_LOCALE() and ~SET_LOCALE() need to actually do something are for systems which don't support per-thread locales, such as (I presume) HAIKU.

If all systems BOINC now supports have per-thread locales, then these functions should always be empty (and actually are no longer needed.) But I'm reluctant to assume that. So I believe that the correct replacement for gui_rpc_client.h lines 776 - 845 should be the following:

#if defined(HAVE_USELOCALE) || defined (HAVE__CONFIGTHREADLOCALE)
struct SET_LOCALE {
    // Don't need to juggle locales if we have per-thread locale
    inline SET_LOCALE() {
    }
    inline ~SET_LOCALE() {
    }
};
#else
    // Use this code for any platforms which do not support 
    // setting locale on a per-thread basis
 struct SET_LOCALE {
    std::string old_locale;
    inline SET_LOCALE() {
        old_locale = setlocale(LC_ALL, NULL);
        setlocale(LC_ALL, "C");
    }
    inline ~SET_LOCALE() {
        setlocale(LC_ALL, old_locale.c_str());
    }
};
#endif

[CharlieFenton]

@JuhaSointusalo I just noticed this:

SET_LOCALE's per-thread locale version has an assumption that it is used
only when the calling thread is already set to use C locale. This
assumption is not documented and not followed in Manager.

I don't understand this. Where is it not followed in the manager? As I wrote above, AsyncRPC.cpp lines 143-144 and 153-154 permanently set the locale for all RPC threads for MSW and Mac, respectively. I should correct this: there is only one RPC thread in the Manager. All RPC requests are queued on this one thread. And this thread is set to use C locale as soon as it begins execution. So SET_LOCALE's per-thread locale version is a pair of empty functions.

[JuhaSointusalo]

Are we still supporting the HAIKU OS? If so, does removing the following code from gui_rpc_client.h break that support?

#if defined(__HAIKU__)
#define NO_PER_THREAD_LOCALE 1
#endif

I don't know if we support Haiku. I just checked the project and it's not exactly dead yet but not strongly alive either.

The code is now driven by configure checks for _configthreadlocale and uselocale or in OS X and Windows cases by predefined check results. If there is any system that can't run configure or doesn't have predefined check results it is essentially assumed to not have per-thread locale support.

I could have kept NO_PER_THREAD_LOCALE but IMHO it made the code harder to read without any benefit.

If all systems BOINC now supports have per-thread locales, then these functions should always be empty (and actually are no longer needed.)

OS X and Windows and probably all still supported Linux distro versions have per-thread locale support but I wasn't sure about other Unixes so I kept the fallback code.

SET_LOCALE's per-thread locale version has an assumption that it is used
only when the calling thread is already set to use C locale. This
assumption is not documented and not followed in Manager.

I don't understand this. Where is it not followed in the manager?

SET_LOCALE is used in CDlgDiagnosticLogFlags::CreateCheckboxes() and CDlgDiagnosticLogFlags::SaveFlags() and these run in GUI thread.

Taking another look at those functions they seem to read/write the XML inside them and not pass it around so maybe they would work with any locale and without SET_LOCALE.

[CharlieFenton]

Taking another look at those functions they seem to read/write the XML inside them and not pass it around so maybe they would work with any locale and without SET_LOCALE.

SET_LOCALE is almost certainly not needed in DlgDiagnosticFlags.cpp. I don't remember why I thought I should include it, but I now believe I was wrong to do so. But note that in systems which support per-thread locale, SET_LOCALE() and ~SET_LOCALE() are empty functions so they do nothing, anyway. And as I explain later in this comment, Linux builds of BOINC already use per-thread locale. The potential problem occurs when NO_PER_THREAD_LOCALE is defined in the following scenario, which is a sort of race condition:
The UI thread sets the "C" locale in CDlgDiagnosticLogFlags::CreateCheckboxes() or CDlgDiagnosticLogFlags::SaveFlags(). Before that method finishes, the RPC thread starts an RPC which calls SET_LOCALE() , so it saves the "C" locale as the "old" locale. Control returns to the UI thread, and the CDlgDiagnosticLogFlags returns, setting the locale back to normal. Later, control returns to the RPC thread, which sets the locale back to the "old" locale, but that is the "C" locale. Now the original locale has been lost. Again, note that this is not an issue on platforms which use per-thread locales.

My current belief that SET_LOCALE should be removed from DlgDiagnosticFlags.cpp is supported by the following entries I found in the old checkin_notes files:

Rom    18 Apr 2006
    - Bug Fix: setlocale is needed in environments where the C runtime library
        will attempt to use the current locales' numerical formating rules to
        extract integer or floating point numbers.  The core client doesn't
        observe local formating rules and defaults to the "C" locale.  When
        parsing data from the CC be sure to flip the locale to "C" and return
        it when your done.

David  18 Apr 2006
    - remove setlocale() calls from parse_int(), parse_double().
        Numbers in XML (including GUI RPCs) are always in standard format.
        Whoever writes XML (e.g. the Manager) must ensure this.

Charlie 15 Aug 2008
    - MGR: async GUI RPCs: Merge GUI RPC code from private workspace into trunk.
        All RPCs now go through separate thread.  There are two categories:

Charlie 25 Mar 2009
    - MGR: Remove erroneous call of locale in main thread which may be causing 
        undesired changes in date and numeric formatting due to conflicts 
        between threads; also set locale only on a per-thread basis if available.

Charlie 26 Mar 2009
    - Mac: revise XCode project to use OS 10.4 SDK for building PowerPC
        libraries and executables so that we can use weak-linking for
        APIs not available before OS 10.4, such as uselocale().
    - MGR: Modify SET_LOCALE constructor and destructor to change locale
        only on those systems without support for setting locale
        on a per-thread basis, such as OS 10.3.9.
    NOTE: At this point it appears that Ubuntu and Fedora both support
        uselocale().  If any platform does not support it, see the 
        comments at NO_PER_THREAD_LOCALE in lib/gui_rpc_client.h for info 
        on allowing for that.

@JuhaSointusalo wrote:

Fix this by making SET_LOCALE per-thread locale aware on Linux.

Note that the last entry above from the checkin_notes implies that Linux builds should not have had NO_PER_THREAD_LOCALE defined, and indeed they do not and never did. So Linux should already be using per-thread locale. As a result, I am skeptical that any of the changes in this PR are needed, or that they will fix the date formatting problem on Linux. Am I missing something?

@JuhaSointusalo: have you actually confirmed that your proposed changes fix the date formatting problem?

[JuhaSointusalo]

Linux builds of BOINC already use per-thread locale

The code makes it look like that but actually it doesn't. This is from config.h (inserted in configure.ac):

#if !HAVE_DECL__CONFIGTHREADLOCALE
#define NO_PER_THREAD_LOCALE 1
#undef HAVE__CONFIGTHREADLOCALE
#else
#undef NO_PER_THREAD_LOCALE
#define HAVE__CONFIGTHREADLOCALE 1
#endif

Linux doesn't have _configthreadlocale and therefore, according to the code, it doesn't have per-thread locales. It was pretty obvious why the time formats were wrong but finding the source of the problem was not so obvious so I think that is a good reason to remove the NO_PER_THREAD_LOCALE and use configure checks.

have you actually confirmed that your proposed changes fix the date formatting problem?

Yes I have. Seeing the wrong time format depends on timing and enough traffic between client and Manager and that's why I wanted @MarkJ62 to verify the fix.

[CharlieFenton]

Thank you for the explanation. Now I understand. This is a problem with configure.ac, DlgDiagnosticFlags.cpp and only those two files. If I understand correctly, _configthreadlocale() is an MSW specific API, so the current (incorrect) code in configure.ac only determines whether the build is on MSW. It seems to me that the correct way to fix this is to fix configure.ac, remove the SET_LOCALE invocations from DlgDiagnosticFlags.cpp, and leave everything else as is. According to GIT blame, this bug was introduced by @SETIguy (Eric Korpela) on 6/20/14 in commit ce37b73 with commit message:

Fixed avx detection on GCC windows compiles
More windows library detection added to configure.ac
Additional cross compile fixes for libboinc and client.

Prior to that, the code read:

#ifndef HAVE_XLOCALE_H
#define NO_PER_THREAD_LOCALE 1
#endif

Since uselocale() is required for per-thread locale support and is declared in the xlocale.h header, I would think that Linux builds of BOINC did use per-thread locale before that commit. That commit would also have broken any Mac builds created using configure & make if they did not use the hand-built config.h.

As far as I can tell, HAVE_DECL__CONFIGTHREADLOCALE and HAVE__CONFIGTHREADLOCALE are defined in lib/boinc_win.h but never used anywhere, and _configthreadlocale() is available only in MSW. Since uselocale() always sets locale on a per-thread basis, the simplest fix is to change that code in configure.ac to the following, remove the SET_LOCALE invocations from DlgDiagnosticFlags.cpp, and make no other changes:

#if !HAVE_USELOCALE
#define NO_PER_THREAD_LOCALE 1
#endif

There is no need to specifically #undef NO_PER_THREAD_LOCALE, since it will be undefined by default. Since uselocale() is generally declared in locale.h, the original code in configure.ac was probably correct. Perhaps @SETIguy can tell us why he felt it needed to be changed.

If BOINC is ever built for MSW using configure and make (which I doubt), and if HAVE__CONFIGTHREADLOCALE is actually used somewhere, and if greater completeness is desired, the following could be used instead:

#undef NO_PER_THREAD_LOCALE
#undef HAVE__CONFIGTHREADLOCALE
#if HAVE_DECL__CONFIGTHREADLOCALE
#define HAVE__CONFIGTHREADLOCALE 1
#elif  !HAVE_USELOCALE
#define NO_PER_THREAD_LOCALE 1
#endif

[CharlieFenton]

Are further changes needed to test for uselocale() or _configthreadlocale() when building using configure and make in order to set HAVE_USELOCALE or HAVE_DECL__CONFIGTHREADLOCALE in config.h? I don't see any reference to either of these anywhere in the GIT source tree.

[CharlieFenton]

On further thought, I am also OK with the proposed changes in your branch, provided you make the modifications to gui_rpc_client.h lines 776 - 845 which I requested here, removing the SET_LOCALE invocations from DlgDiagnosticFlags.cpp and making any necessary changes to ensure that configure defines or undefines HAVE_USELOCALE properly in config.h. This does clean the code up significantly.

Removing the HAIKU specific code is probably OK, assuming that the HAIKU build will use configure and make.

[JuhaSointusalo]

Removing SET_LOCALE from DlgDiagnosticsFlags.cpp and reverting to no-op per-thread locale SET_LOCALE sounds like a good plan. I'll keep the code clean up changes. I never miss an easy opportunity to do code clean up.

If BOINC is ever built for MSW using configure and make (which I doubt)

Well... I want to update Windows build system to VS2017 which doesn't run on XP, you want people to be able to hack BOINC on their computers, including XP users. One way for both of us to get what we want is to tell XP users to use Autotools+MinGW. Autotools builds don't actually work right now on Windows but I'm prepared to fix that.

FWIW:

Prior to that, the code read:

#ifndef HAVE_XLOCALE_H
#define NO_PER_THREAD_LOCALE 1
#endif

That code is actually wrong too today. POSIX 2008 says uselocale is declared in locale.h. xlocale.h is a non-standard extension and was recently removed from GNU libc. Back when per-thread locales was still experimental feature it might have been declared in xlocale.h and maybe some systems still have it that way. So uselocale really needs a proper configure check (which is what this PR adds).

[CharlieFenton]

Thank you for tracking down the obscure reason for this long-standing bug and finding a fix for it. And thank you for the very worthwhile discussion. I now realize that, even without the SET_LOCALE invocations within DlgDiagnosticsFlags.cpp, the date & time formatting issue will be present in any system without per-thread locales. The "C" locale will be used any time the UI thread formats a date & time while an RPC is in progress in the RPC thread.

I don't see any way to fix that except to not use asynchronous RPCs in any system without per-thread locale; in other words, not to use a separate RPC thread in those systems. That seems like an unacceptable solution, not just because it requires major code changes but also because of the original motivation for adding asynchronous RPC support: without the separate RPC thread, the UI can become unresponsive when an RPC takes a long time.

My only other idea would be to change the locale in the client to match that of the Manager, eliminating the need to use SET_LOCALE in RPCs. But that would break backward compatibility when connecting the Manager to an older client or vice-versa.

Do you have any other solutions for systems without per-thread locale?

[JuhaSointusalo]

Do you have any other solutions for systems without per-thread locale?

Since we'd need to protect the global locale from being changed from one thread while another thread is using it then add a mutex and have RPC code and Manager's time formatting functions grab it. Limiting the time the mutex is held for only writing requests and reading replies and releasing it while the requests and replies are in transit would prevent the Manager from becoming unresponsive.

But I think that is still kinda ugly considering it's not absolutely necessary. I would have thought the problem shows in Manager's Task tab too but I only see it in Event Log. And while the problem shows up with only a few log messages every now and then the best way to see it is to have lots of messages frequently. That means either some debug flag is enabled or it's a high-throughput host. A high-throughput host (i.e. GPUs) most likely supports per-thread locales.

All this limits the number of people who see the problem and the frequency it's seen to the level that I think it's not worth doing anything about it.

[TheAspens]

@CharlieFenton and @JuhaSointusalo - have you guys reached a consensus that this work is complete? I think Richard would like to include this in 7.9.4/7.10.1 build (depending on if the next build is a release candidate build or not). If you guys feel comfortable with it, then @CharlieFenton please merge.

[CharlieFenton]

I believe @JuhaSointusalo has not yet made the changes to his feature branch which we agreed on:

make the modifications to gui_rpc_client.h lines 776 - 845 which I requested here, removing the SET_LOCALE invocations from DlgDiagnosticFlags.cpp and making any necessary changes to ensure that configure defines or undefines HAVE_USELOCALE properly in config.h.

@JuhaSointusalo: please let me know when you have done that, and I will be happy to merge.

[JuhaSointusalo]

Ready for next review. I also updated commit messages to reflect the discussion here.

[MarkJ62]

Hi Juha, Sorry about the delay on this one. I just discovered your message in my spam folder. I have downloaded it, will give a go in the next few minutes. CheersMarkJ From: Juha Sointusalo <notifications@github.com> To: BOINC/boinc <boinc@noreply.github.com> Cc: MarkJ62 <tarotapprentice@yahoo.com>; Mention <mention@noreply.github.com> Sent: Friday, 23 March 2018, 5:22 Subject: Re: [BOINC/boinc] lib, mgr: use per-thread locale on Linux (#2420) @MarkJ62 Please test: https://drive.google.com/file/d/1N3ygPLzxgmZoRvXFzAhKgG02HNLakY4u/view?usp=sharing@CharlieFenton I removed OS X < 10.4 code because you don't support it. Right? Please check I didn't break the Mac build too much.— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.