[GNC-dev] gnucash maint: Multiple changes pushed

[Geert Janssens]

Op vrijdag 11 januari 2019 01:42:41 CET schreef 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#Ini
> tialization. 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. ;-)

Yeah, we'll probably have a clearer picture then.

Geert