Re: [GNC-dev] gnucash maint: Multiple changes pushed

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
Op dinsdag 1 januari 2019 22:14:16 CET schreef John Ralls:

> Updated via  https://github.com/Gnucash/gnucash/commit/3a105f07 (commit)
> via  https://github.com/Gnucash/gnucash/commit/95bee405 (commit)
> via  https://github.com/Gnucash/gnucash/commit/cc3bb4ef (commit)
> from  https://github.com/Gnucash/gnucash/commit/0f53b6c8 (commit)
>
>
>
> commit 3a105f0728984df7f063110acc8390c93722d581
> Author: John Ralls <[hidden email]>
> Date:   Tue Jan 1 13:12:39 2019 -0800
>
>     Catch boost::locale character-conversion exceptions.
>
>     Partial cause of the crash reported in Bug 797002.
>
I suppose you meant 796996 ?

Also it looks like you're really only catching the errors. The source of the
conversion issue itself is not really determined yet ?

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
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 ?

Any idea how difficult would it be to fix that ?

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.

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Derek Atkins-3
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.

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.

-derek

--
       Derek Atkins                 617-623-3745
       [hidden email]             www.ihtfp.com
       Computer and Internet Security Consultant

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

John Ralls-2


> On Jan 9, 2019, at 6:17 AM, Derek Atkins <[hidden email]> 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.
>
> 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.

Regards,
John Ralls

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

John Ralls-2
In reply to this post by Geert Janssens-4


> On Jan 9, 2019, at 4:24 AM, Geert Janssens <[hidden email]> wrote:
>
> Op dinsdag 1 januari 2019 22:14:16 CET schreef John Ralls:
>> Updated via  https://github.com/Gnucash/gnucash/commit/3a105f07 (commit)
>> via  https://github.com/Gnucash/gnucash/commit/95bee405 (commit)
>> via  https://github.com/Gnucash/gnucash/commit/cc3bb4ef (commit)
>> from  https://github.com/Gnucash/gnucash/commit/0f53b6c8 (commit)
>>
>>
>>
>> commit 3a105f0728984df7f063110acc8390c93722d581
>> Author: John Ralls <[hidden email]>
>> Date:   Tue Jan 1 13:12:39 2019 -0800
>>
>>    Catch boost::locale character-conversion exceptions.
>>
>>    Partial cause of the crash reported in Bug 797002.
>>
> I suppose you meant 796996 ?
>
> Also it looks like you're really only catching the errors. The source of the
> conversion issue itself is not really determined yet ?

Yes, wrong bug.

Yes, in this one I’m only catching the exceptions because uncaught exceptions cause crashes. The root cause was libc’s not-quite-right creation of e.g. “Spanish_Spain.1252” locale strings, the .1252 part choking gen(“”) (and even Spanish_Spain chokes std::locale(“”). That’s addressed with the more thorough error handling and use of gen(“”) in b4fedff90e.

Regards,
John Ralls

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
In reply to this post by John Ralls-2
Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls:

> > On Jan 9, 2019, at 6:17 AM, Derek Atkins <[hidden email]> 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


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
In reply to this post by John Ralls-2
Op woensdag 9 januari 2019 15:57:07 CET schreef John Ralls:
> > On Jan 9, 2019, at 4:24 AM, Geert Janssens <[hidden email]>
> > wrote:>
> > Op dinsdag 1 januari 2019 22:14:16 CET schreef John Ralls:
> >> Updated via  https://github.com/Gnucash/gnucash/commit/3a105f07 
(commit)

> >>
> >> via  https://github.com/Gnucash/gnucash/commit/95bee405 (commit)
> >> via  https://github.com/Gnucash/gnucash/commit/cc3bb4ef (commit)
> >>
> >> from  https://github.com/Gnucash/gnucash/commit/0f53b6c8 (commit)
> >>
> >> commit 3a105f0728984df7f063110acc8390c93722d581
> >> Author: John Ralls <[hidden email]>
> >> Date:   Tue Jan 1 13:12:39 2019 -0800
> >>
> >>    Catch boost::locale character-conversion exceptions.
> >>    
> >>    Partial cause of the crash reported in Bug 797002.
> >
> > I suppose you meant 796996 ?
> >
> > Also it looks like you're really only catching the errors. The source of
> > the conversion issue itself is not really determined yet ?
>
> Yes, wrong bug.
>
> Yes, in this one I’m only catching the exceptions because uncaught
> exceptions cause crashes. The root cause was libc’s not-quite-right
> creation of e.g. “Spanish_Spain.1252” locale strings, the .1252 part
> choking gen(“”) (and even Spanish_Spain chokes std::locale(“”). That’s
> addressed with the more thorough error handling and use of gen(“”) in
> b4fedff90e.
>
> Regards,
> John Ralls

Oh right. I saw that commit later on, but didn't make the connection. Thanks.

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
Op woensdag 9 januari 2019 18:06:49 CET schreef Geert Janssens:

> Op woensdag 9 januari 2019 15:57:07 CET schreef John Ralls:
> > > On Jan 9, 2019, at 4:24 AM, Geert Janssens <[hidden email]>
> > > wrote:>
> > >
> > > Op dinsdag 1 januari 2019 22:14:16 CET schreef John Ralls:
> > >> Updated via  https://github.com/Gnucash/gnucash/commit/3a105f07
>
> (commit)
>
> > >> via  https://github.com/Gnucash/gnucash/commit/95bee405 (commit)
> > >> via  https://github.com/Gnucash/gnucash/commit/cc3bb4ef (commit)
> > >>
> > >> from  https://github.com/Gnucash/gnucash/commit/0f53b6c8 (commit)
> > >>
> > >> commit 3a105f0728984df7f063110acc8390c93722d581
> > >> Author: John Ralls <[hidden email]>
> > >> Date:   Tue Jan 1 13:12:39 2019 -0800
> > >>
> > >>    Catch boost::locale character-conversion exceptions.
> > >>    
> > >>    Partial cause of the crash reported in Bug 797002.
> > >
> > > I suppose you meant 796996 ?
> > >
> > > Also it looks like you're really only catching the errors. The source of
> > > the conversion issue itself is not really determined yet ?
> >
> > Yes, wrong bug.
> >
> > Yes, in this one I’m only catching the exceptions because uncaught
> > exceptions cause crashes. The root cause was libc’s not-quite-right
> > creation of e.g. “Spanish_Spain.1252” locale strings, the .1252 part
> > choking gen(“”) (and even Spanish_Spain chokes std::locale(“”). That’s
> > addressed with the more thorough error handling and use of gen(“”) in
> > b4fedff90e.
> >
> > Regards,
> > John Ralls
>
> Oh right. I saw that commit later on, but didn't make the connection.
> Thanks.

Returning to this: it occurred to me bug 796996 is about a crash on MacOS,
though the example you give looks like a Windows locale name.

Does libc MacOS also create improper locale names and does b4fedff90e fix this
as well ?

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

John Ralls-2


> On Jan 9, 2019, at 11:52 AM, Geert Janssens <[hidden email]> wrote:
>
> Op woensdag 9 januari 2019 18:06:49 CET schreef Geert Janssens:
>> Op woensdag 9 januari 2019 15:57:07 CET schreef John Ralls:
>>>> On Jan 9, 2019, at 4:24 AM, Geert Janssens <[hidden email]>
>>>> wrote:>
>>>>
>>>> Op dinsdag 1 januari 2019 22:14:16 CET schreef John Ralls:
>>>>> Updated via  https://github.com/Gnucash/gnucash/commit/3a105f07
>>
>> (commit)
>>
>>>>> via  https://github.com/Gnucash/gnucash/commit/95bee405 (commit)
>>>>> via  https://github.com/Gnucash/gnucash/commit/cc3bb4ef (commit)
>>>>>
>>>>> from  https://github.com/Gnucash/gnucash/commit/0f53b6c8 (commit)
>>>>>
>>>>> commit 3a105f0728984df7f063110acc8390c93722d581
>>>>> Author: John Ralls <[hidden email]>
>>>>> Date:   Tue Jan 1 13:12:39 2019 -0800
>>>>>
>>>>>   Catch boost::locale character-conversion exceptions.
>>>>>
>>>>>   Partial cause of the crash reported in Bug 797002.
>>>>
>>>> I suppose you meant 796996 ?
>>>>
>>>> Also it looks like you're really only catching the errors. The source of
>>>> the conversion issue itself is not really determined yet ?
>>>
>>> Yes, wrong bug.
>>>
>>> Yes, in this one I’m only catching the exceptions because uncaught
>>> exceptions cause crashes. The root cause was libc’s not-quite-right
>>> creation of e.g. “Spanish_Spain.1252” locale strings, the .1252 part
>>> choking gen(“”) (and even Spanish_Spain chokes std::locale(“”). That’s
>>> addressed with the more thorough error handling and use of gen(“”) in
>>> b4fedff90e.
>>>
>>> Regards,
>>> John Ralls
>>
>> Oh right. I saw that commit later on, but didn't make the connection.
>> Thanks.
>
> Returning to this: it occurred to me bug 796996 is about a crash on MacOS,
> though the example you give looks like a Windows locale name.
>
> Does libc MacOS also create improper locale names and does b4fedff90e fix this
> as well ?

Yes, and so do some Linux distros. What chokes gen(“”) is the appended encoding.  That’s why b4fedff90e strips everything after ‘.’ in the locale string and tries again.

The wider Windows case came up on IRC: https://wiki.gnucash.org/logs/2019/01/04.html#T15:58:41 <https://wiki.gnucash.org/logs/2019/01/04.html#T15:58:41>. That fail was from std::locale and is what led me to use gen(“”) for all C++ locale retrievals.

Regards,
John Ralls

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

John Ralls-2
In reply to this post by Geert Janssens-4


> On Jan 9, 2019, at 9:06 AM, Geert Janssens <[hidden email]> wrote:
>
> Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls:
>>> On Jan 9, 2019, at 6:17 AM, Derek Atkins <[hidden email]> 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.

Regards,
John Ralls

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
In reply to this post by John Ralls-2
Op donderdag 10 januari 2019 02:08:29 CET schreef John Ralls:

> > On Jan 9, 2019, at 11:52 AM, Geert Janssens <[hidden email]>
> > wrote:>
> > Op woensdag 9 januari 2019 18:06:49 CET schreef Geert Janssens:
> >> Op woensdag 9 januari 2019 15:57:07 CET schreef John Ralls:
> >>>> On Jan 9, 2019, at 4:24 AM, Geert Janssens <[hidden email]>
> >>>> wrote:>
> >>>>
> >>>> Op dinsdag 1 januari 2019 22:14:16 CET schreef John Ralls:
> >>>>> Updated via  https://github.com/Gnucash/gnucash/commit/3a105f07
> >>
> >> (commit)
> >>
> >>>>> via  https://github.com/Gnucash/gnucash/commit/95bee405 (commit)
> >>>>> via  https://github.com/Gnucash/gnucash/commit/cc3bb4ef (commit)
> >>>>>
> >>>>> from  https://github.com/Gnucash/gnucash/commit/0f53b6c8 (commit)
> >>>>>
> >>>>> commit 3a105f0728984df7f063110acc8390c93722d581
> >>>>> Author: John Ralls <[hidden email]>
> >>>>> Date:   Tue Jan 1 13:12:39 2019 -0800
> >>>>>
> >>>>>   Catch boost::locale character-conversion exceptions.
> >>>>>  
> >>>>>   Partial cause of the crash reported in Bug 797002.
> >>>>
> >>>> I suppose you meant 796996 ?
> >>>>
> >>>> Also it looks like you're really only catching the errors. The source
> >>>> of
> >>>> the conversion issue itself is not really determined yet ?
> >>>
> >>> Yes, wrong bug.
> >>>
> >>> Yes, in this one I’m only catching the exceptions because uncaught
> >>> exceptions cause crashes. The root cause was libc’s not-quite-right
> >>> creation of e.g. “Spanish_Spain.1252” locale strings, the .1252 part
> >>> choking gen(“”) (and even Spanish_Spain chokes std::locale(“”). That’s
> >>> addressed with the more thorough error handling and use of gen(“”) in
> >>> b4fedff90e.
> >>>
> >>> Regards,
> >>> John Ralls
> >>
> >> Oh right. I saw that commit later on, but didn't make the connection.
> >> Thanks.
> >
> > Returning to this: it occurred to me bug 796996 is about a crash on MacOS,
> > though the example you give looks like a Windows locale name.
> >
> > Does libc MacOS also create improper locale names and does b4fedff90e fix
> > this as well ?
>
> Yes, and so do some Linux distros. What chokes gen(“”) is the appended
> encoding.  That’s why b4fedff90e strips everything after ‘.’ in the locale
> string and tries again.
>
> The wider Windows case came up on IRC:
> https://wiki.gnucash.org/logs/2019/01/04.html#T15:58:41
> <https://wiki.gnucash.org/logs/2019/01/04.html#T15:58:41>. That fail was
> from std::locale and is what led me to use gen(“”) for all C++ locale
> retrievals.
>
Ok, thanks for the clarifications.

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
In reply to this post by John Ralls-2
Op donderdag 10 januari 2019 06:45:15 CET schreef John Ralls:

> > On Jan 9, 2019, at 9:06 AM, Geert Janssens <[hidden email]>
> > wrote:>
> > Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls:
> >>> On Jan 9, 2019, at 6:17 AM, Derek Atkins <[hidden email]> 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.

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

John Ralls-2


> On Jan 10, 2019, at 2:12 AM, Geert Janssens <[hidden email]> wrote:
>
> Op donderdag 10 januari 2019 06:45:15 CET schreef John Ralls:
>>> On Jan 9, 2019, at 9:06 AM, Geert Janssens <[hidden email]>
>>> wrote:>
>>> Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls:
>>>>> On Jan 9, 2019, at 6:17 AM, Derek Atkins <[hidden email]> 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. 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.

Regards,
John Ralls


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
Op donderdag 10 januari 2019 16:35:57 CET schreef John Ralls:

> > On Jan 10, 2019, at 2:12 AM, Geert Janssens <[hidden email]>
> > wrote:>
> > Op donderdag 10 januari 2019 06:45:15 CET schreef John Ralls:
> >>> On Jan 9, 2019, at 9:06 AM, Geert Janssens <[hidden email]>
> >>> wrote:>
> >>>
> >>> Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls:
> >>>>> On Jan 9, 2019, at 6:17 AM, Derek Atkins <[hidden email]> 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.

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

John Ralls-2


> On Jan 10, 2019, at 8:32 AM, Geert Janssens <[hidden email]> wrote:
>
> Op donderdag 10 januari 2019 16:35:57 CET schreef John Ralls:
>>> On Jan 10, 2019, at 2:12 AM, Geert Janssens <[hidden email]>
>>> wrote:>
>>> Op donderdag 10 januari 2019 06:45:15 CET schreef John Ralls:
>>>>> On Jan 9, 2019, at 9:06 AM, Geert Janssens <[hidden email]>
>>>>> wrote:>
>>>>>
>>>>> Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls:
>>>>>>> On Jan 9, 2019, at 6:17 AM, Derek Atkins <[hidden email]> 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

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
Op donderdag 10 januari 2019 21:11:26 CET schreef John Ralls:

> > On Jan 10, 2019, at 8:32 AM, Geert Janssens <[hidden email]>
> > wrote:>
> > Op donderdag 10 januari 2019 16:35:57 CET schreef John Ralls:
> >>> On Jan 10, 2019, at 2:12 AM, Geert Janssens <[hidden email]>
> >>> wrote:>
> >>>
> >>> Op donderdag 10 januari 2019 06:45:15 CET schreef John Ralls:
> >>>>> On Jan 9, 2019, at 9:06 AM, Geert Janssens
> >>>>> <[hidden email]>
> >>>>> wrote:>
> >>>>>
> >>>>> Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls:
> >>>>>>> On Jan 9, 2019, at 6:17 AM, Derek Atkins <[hidden email]> 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.

Geert


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

John Ralls-2


> On Jan 10, 2019, at 12:44 PM, Geert Janssens <[hidden email]> wrote:
>
> Op donderdag 10 januari 2019 21:11:26 CET schreef John Ralls:
>>> On Jan 10, 2019, at 8:32 AM, Geert Janssens <[hidden email]>
>>> wrote:>
>>> Op donderdag 10 januari 2019 16:35:57 CET schreef John Ralls:
>>>>> On Jan 10, 2019, at 2:12 AM, Geert Janssens <[hidden email]>
>>>>> wrote:>
>>>>>
>>>>> Op donderdag 10 januari 2019 06:45:15 CET schreef John Ralls:
>>>>>>> On Jan 9, 2019, at 9:06 AM, Geert Janssens
>>>>>>> <[hidden email]>
>>>>>>> wrote:>
>>>>>>>
>>>>>>> Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls:
>>>>>>>>> On Jan 9, 2019, at 6:17 AM, Derek Atkins <[hidden email]> 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

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Derek Atkins-3

On Thu, January 10, 2019 7:42 pm, John Ralls wrote:
>
[snip]

>
> 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. ;-)

Add a comment in the code to remind us?

> Regards,
> John Ralls

-derek

--
       Derek Atkins                 617-623-3745
       [hidden email]             www.ihtfp.com
       Computer and Internet Security Consultant

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

John Ralls-2


> On Jan 10, 2019, at 5:25 PM, Derek Atkins <[hidden email]> wrote:
>
>
> On Thu, January 10, 2019 7:42 pm, John Ralls wrote:
>>
> [snip]
>>
>> 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. ;-)
>
> Add a comment in the code to remind us?

There's nothing special about it. GnuCash is written in a way that pretty much everything except function local variables is global thanks to QofContainer. Transactions are lightly protected in the obvious case where one might try to edit the same one in two registers, but that's about it. qof_begin_edit/qof_commit_edit afford no protection at all... quite the opposite, in fact.

Regards,
John Ralls

_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel
Reply | Threaded
Open this post in threaded view
|

Re: [GNC-dev] gnucash maint: Multiple changes pushed

Geert Janssens-4
In reply to this post by John Ralls-2
Op vrijdag 11 januari 2019 01:42:41 CET schreef John Ralls:

> > On Jan 10, 2019, at 12:44 PM, Geert Janssens <[hidden email]>
> > wrote:>
> > Op donderdag 10 januari 2019 21:11:26 CET schreef John Ralls:
> >>> On Jan 10, 2019, at 8:32 AM, Geert Janssens <[hidden email]>
> >>> wrote:>
> >>>
> >>> Op donderdag 10 januari 2019 16:35:57 CET schreef John Ralls:
> >>>>> On Jan 10, 2019, at 2:12 AM, Geert Janssens
> >>>>> <[hidden email]>
> >>>>> wrote:>
> >>>>>
> >>>>> Op donderdag 10 januari 2019 06:45:15 CET schreef John Ralls:
> >>>>>>> On Jan 9, 2019, at 9:06 AM, Geert Janssens
> >>>>>>> <[hidden email]>
> >>>>>>> wrote:>
> >>>>>>>
> >>>>>>> Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls:
> >>>>>>>>> On Jan 9, 2019, at 6:17 AM, Derek Atkins <[hidden email]> 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


_______________________________________________
gnucash-devel mailing list
[hidden email]
https://lists.gnucash.org/mailman/listinfo/gnucash-devel