[GNC-dev] gnucash maint: Multiple changes pushed

[Geert Janssens]

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.

Geert