-
Notifications
You must be signed in to change notification settings - Fork 444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib, mgr: use per-thread locale on Linux #2420
lib, mgr: use per-thread locale on Linux #2420
Conversation
@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. |
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?
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:
|
@JuhaSointusalo I just noticed this:
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. |
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 I could have kept
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.
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 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: 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:
@JuhaSointusalo wrote:
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? |
The code makes it look like that but actually it doesn't. This is from
Linux doesn't have
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. |
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,
Prior to that, the code read:
Since As far as I can tell,
There is no need to specifically If BOINC is ever built for MSW using configure and make (which I doubt), and if
|
Are further changes needed to test for |
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 Removing the HAIKU specific code is probably OK, assuming that the HAIKU build will use configure and make. |
Removing
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:
That code is actually wrong too today. POSIX 2008 says |
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 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 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. |
@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. |
I believe @JuhaSointusalo has not yet made the changes to his feature branch which we agreed on:
@JuhaSointusalo: please let me know when you have done that, and I will be happy to merge. |
Allows fixing and cleaning up per-thread locale support in Manager and libboinc. locale.h and xlocale.h were checked for libboinc_graphics. Move xlocale.h check to correct place and remove locale.h check. locale.h has been part of C standard library since C89. The support for per-thread locales cannot be reliably inferred from the existence of different headers. Some systems declare uselocale() in locale.h, others in xlocale.h and xlocale.h is no longer included in GNU libc. Instead explicitly check for uselocale() and _configthreadlocale(). Add uselocale() check result to Mac config.h so that the #ifdef mazes can be simplified. Also correct quoting in AC_CHECK_FUNCS and AC_CHECK_HEADERS calls.
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. Use SET_LOCALE's per-thread locale version and set thread locale in Manager's RPC thread to fix this. Also clean up #ifdef mazes in SET_LOCALE and Manager's RPC thread now that there are HAVE_* macros available. Remove OS X < 10.4 code because that old OS X versions are not supported any more. Fixes #2399.
SET_LOCALE is no-op when using per-thread locales making SET_LOCALE unnecessary. When per-thread locales are not available using SET_LOCALE in CDlgDiagnosticLogFlags can cause Manager to get stuck to C locale. The lifetime of two SET_LOCALE objects can be interleaved if log flags are loaded or saved and an async RPC is launched at the same time. If the lifetimes are interleaved the first object sets global locale to C. The second object saves the global locale that was set to C by the first object. First object restores the global locale correctly. After that the second object incorrectly restores the global locale to C.
NO_PER_THREAD_LOCALE has been replaced by HAVE__CONFIGTHREADLOCALE and HAVE_USELOCALE. Also remove HAVE_DECL__CONFIGTHREADLOCALE which was used only for NO_PER_THREAD_LOCALE.
Ready for next review. I also updated commit messages to reflect the discussion here. |
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.
|
lib, mgr: use per-thread locale on Linux
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.