[GNC-dev] gnucash maint: Multiple changes pushed

[John Ralls]

> On Jan 10, 2019, at 8:32 AM, Geert Janssens <geert.gnucash at kobaltwit.be> wrote:
> 
> Op donderdag 10 januari 2019 16:35:57 CET schreef John Ralls:
>>> On Jan 10, 2019, at 2:12 AM, Geert Janssens <geert.gnucash at kobaltwit.be>
>>> wrote:> 
>>> Op donderdag 10 januari 2019 06:45:15 CET schreef John Ralls:
>>>>> On Jan 9, 2019, at 9:06 AM, Geert Janssens <geert.gnucash at kobaltwit.be>
>>>>> wrote:>
>>>>> 
>>>>> Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls:
>>>>>>> On Jan 9, 2019, at 6:17 AM, Derek Atkins <derek at ihtfp.com> wrote:
>>>>>>> 
>>>>>>> HI,
>>>>>>> 
>>>>>>> On Wed, January 9, 2019 9:12 am, Geert Janssens wrote:
>>>>>>>> I like the idea of caching the system locale for future use. Too bad
>>>>>>>> the
>>>>>>>> std::locale is working so poorly on Windows :(
>>>>>>>> 
>>>>>>>> Op zondag 6 januari 2019 19:13:28 CET schreef John Ralls:
>>>>>>>>> +const std::locale&
>>>>>>>>> +gnc_get_locale()
>>>>>>>>> +{
>>>>>>>>> +  static std::locale cached;
>>>>>>>>> +  static bool tried_already = false;
>>>>>>>> 
>>>>>>>> If I understand it correctly using static variables makes the
>>>>>>>> function
>>>>>>>> unsuitable for multi-threading, right ?
>>>>>>> 
>>>>>>> Not necessarily.  There is a race condition on first-use, but beyond
>>>>>>> that
>>>>>>> I don't see a MT issue here.  The data is read-only, right?  Multiple
>>>>>>> threads could read from the same read-only data simultaneously, so
>>>>>>> that
>>>>>>> should be fine.
>>>>> 
>>>>> It was this first-use race condition I was referring to.
>>>>> 
>>>>> Particularly, for thread safety I think setting tried_already = true
>>>>> should
>>>>> happen at the very end of the function, after we're sure cached has a
>>>>> proper value. Otherwise a competing thread could try to get the
>>>>> uninitialized cached value if thread execution of the first thread is
>>>>> interrupted right after tried_already is set true.
>>>>> 
>>>>>>> Static data is ont MT-unsafe if it's being changed on a per-call basis
>>>>>>> (e.g. a time_t -> string API returning a static string buffer).
>>>>>>> 
>>>>>>>> Any idea how difficult would it be to fix that ?
>>>>>>> 
>>>>>>> You could add a mutex around the initialization.  That's all I think
>>>>>>> you
>>>>>>> would need here.
>>>>>>> 
>>>>>>>> I know GnuCash is not thread-safe by a long shot and gtk itself is
>>>>>>>> single
>>>>>>>> threaded so it doesn't matter that much.
>>>>>>>> 
>>>>>>>> However I silently set a personal goal of changing that sometime. The
>>>>>>>> C
>>>>>>>> code
>>>>>>>> is a lost cause IMO, but it might be worth to keep multi-threading in
>>>>>>>> mind
>>>>>>>> as
>>>>>>>> we gradually convert to c++. In my basic understanding of threading
>>>>>>>> this
>>>>>>>> particular function should not be too hard to make tread safe.
>>>>>> 
>>>>>> Right, and I decided against making this the first introduction of
>>>>>> mutex
>>>>>> into GnuCash. I think std::atomic
>>>>>> (https://en.cppreference.com/w/cpp/atomic/atomic
>>>>>> <https://en.cppreference.com/w/cpp/atomic/atomic>) is the correct
>>>>>> modern
>>>>>> C++ way for a simple case like this.
>>>>> 
>>>>> I was hoping indeed modern C++ has some answer to this. This is my first
>>>>> foray into multi-threading by the way so I'm pretty green in this area
>>>>> and wishing to learn more about it.
>>>>> 
>>>>> In this particular case would declaring the cached and tried_already as
>>>>> atomic be sufficient to make this thread safe ?
>>>>> 
>>>>> It seems to me this would still allow multiple threads to go in and run
>>>>> the
>>>>> initialization code for setting cached. Given they all should end up
>>>>> setting cached to the same locale it's probably not dangerous to the
>>>>> application, only potentially running the same code multiple times where
>>>>> only once would be sufficient.
>>>> 
>>>> My knowledge of concurrency is pretty dated. I had an operating system
>>>> course 30 years ago that included a discussion of concurrency, and some
>>>> parts of Glib are thread-safe, so I’ve bounced up against it doing
>>>> maintenance a few times. Just mutex and semaphore stuff.
>>>> 
>>>> I haven’t even yet read the concurrency chapter in Josuttis, but my
>>>> understanding of std::atomic is that a std::atomic variable magically
>>>> eliminates races and has certain operations (construction and operator=
>>>> among them) that are guaranteed to be, well, atomic: The compiler will
>>>> always create a contiguous uninterruptible sequence of machine
>>>> instructions
>>>> for atomic operations. There are also ways to order execution of some
>>>> functions with std::memory_model that can make locks (i.e. mutexes and
>>>> semaphores) unnecessary, and the compiler is able to figure out when
>>>> that’s
>>>> true and when it isn’t and to take care of the locking without the
>>>> programmer needing to explicitly code it.
>>>> 
>>>> It’s not something I think worth worrying about now. GnuCash is very far
>>>> away from being multi-threaded. Besides, the natural home for the
>>>> instantiation of the cached C++ local is in main(), after all of the
>>>> environment options are parsed but before the GUI is loaded or the first
>>>> session started. This isn’t the function to worry about concurrent
>>>> access.
>>> 
>>> Fair enough. Though I'd think it should happen in libgnucash
>>> initialization at some point rather than in the gui code. But that's just
>>> a matter of sorting out all startup configuration we do at some point and
>>> estimate which part is needed for libgnucash and which part is only for
>>> the gui code.
>> 
>> Um, I don’t think it’s right to consider main() part of “the GUI code”. It’s
>> the mandatory starting function for any program.
> 
> Except when we want to interface with libgnucash from binding languages such 
> as python or guile directly. At least in this context there's no main() that 
> can explicitly set our libgnucash environmental requirements (like environment 
> variables from the environment file or locale etc) that I know of.
> 
> GUI was indeed a vague term to explain this.
> 
> You could argue the init function itself could be exported so the bindings can 
> call that as well, making the calling script the equivalent of the main() 
> function. That would be a valid option.
> 
>> There’s at present no
>> “libgnucash initialization” at runtime unless you mean
>> libgncmod_engine_init. We can make one, of course, or extend
>> libgncmod_engine_init, but that’s still going to be called from main() (as
>> libgncmod_engine_init is now, via gnc_module_init) and should happen before
>> a future thread-enabled GnuCash starts spawning threads.
>> 
> My message had many implicit assumptions. I'm aware there's no formal 
> libgnucash initialization yet. This was thinking out loud about the future, a 
> future libgnucash where we no longer depend on gnc-module (which is one of our 
> long-term plans).
> 
> At that point we'll need some form of libgnucash initalization and yes, 
> libgncmod_engine_init would be a first potential candidate.
> 
> A more radical idea (not fully analyzed by any means) could be to build up 
> libgnucash as a class hierarchy in which the constructor of the top-level 
> class does all the more general initialization (like configuring the 
> environment and locale).
> In that case initialization would happen automatically at first instantiation 
> of that class. That could happen in a main() function for a C++ program (like 
> future GnuCash proper) or from a binding script.

I don't think that writers of other programs would be happy to have libgnucash doing its own localization determination separate from their program, and especially not changing the environment behind their backs.

The portable way for localization is for the calling program to do whatever it wants to set the locale in the environment and for dependencies to read the environment if they need to. That's not to say that we couldn't move our localization code to core-utils and expose it in the libgnucash API, but it shouldn't happen automatically in case the application wants to do something different.

Regards,
John Ralls