[GNC-dev] gnucash maint: Multiple changes pushed

[John Ralls]

> On Jan 10, 2019, at 12:44 PM, Geert Janssens <geert.gnucash at kobaltwit.be> wrote:
> 
> Op donderdag 10 januari 2019 21:11:26 CET schreef 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.
> 
> I don't think what I am trying to convey would do anything you worry about. 
> Let's refine my thoughts a bit more then.
> 
> I would like libgnucash to "just work" by default and not force calling code 
> to do initialization unless they want to deviate from that default. That means 
> reading necessary bits from the environment, which we only want to do once. So 
> it makes sense to do so at the very first use of anything libgnucash in my 
> opinion.
> I see similar things in for example libgtk. It behaves sensible by default but 
> it can be tweaked by means of a settings.ini file or some environment 
> variables.
> 
> GnuCash as is currently stands already depends on environment in several ways: 
> locale is one, paths to dbi drivers is another, as are load paths for guile. 
> Most of these will also be required to make libgnucash function properly when 
> we are ready to split it off. So I think it would be useful if libgnucash 
> could manage a default configuration for these things with an exposed 
> interface to allow callers to override these (be it via a configuration file, 
> environment variables, direct function calls,... to be detailed).
> 
> So the idea is that a properly installed libgnucash in a well defined 
> environment is able to initialize itself. If overrides are needed, calling 
> programs should be able to provide those. That should make setting 
> configuration of libgnucash optional in the best case and easy in the worst.
> 
> Note my idea of implicitly initializing on first use probably implies any 
> overrides should be set in some way *before* that first use. So either in a 
> config file, via the environment or via optional parameters to the 
> constructor. The latter is probably not very feasible.
> 

Gtk has gtk_init. You call it, set up your GUI, then call gtk_main to start the event loop. libguile has several initialization functions to choose from: https://www.gnu.org/software/guile/manual/html_node/Initialization.html#Initialization.
No "initialize on first use" there.

Consider that "initialize on first use" means that *every* exported function has to have a clause or belong to a class whose constructor checks for and initializes the global state. If we just need to do it for localizing the C++ UI locale, that's what get_cpp_locale() does and we're done, except that we'd have to make that first call thread safe. If we need to do much more, a library init function is cleaner and simpler... and might be cleaner and simpler than making get_cpp_locale thread safe.

This is all for the rather distant future, though, so let's defer figuring it out til then. We'll forget by then anything we decide now. ;-)

Regards,
John Ralls