Op vrijdag 11 januari 2019 01:42:41 CET schreef John Ralls: > > On Jan 10, 2019, at 12:44 PM, Geert Janssens <geert.gnuc...@kobaltwit.be> > > wrote:> > > Op donderdag 10 januari 2019 21:11:26 CET schreef John Ralls: > >>> On Jan 10, 2019, at 8:32 AM, Geert Janssens <geert.gnuc...@kobaltwit.be> > >>> wrote:> > >>> > >>> Op donderdag 10 januari 2019 16:35:57 CET schreef John Ralls: > >>>>> On Jan 10, 2019, at 2:12 AM, Geert Janssens > >>>>> <geert.gnuc...@kobaltwit.be> > >>>>> wrote:> > >>>>> > >>>>> Op donderdag 10 januari 2019 06:45:15 CET schreef John Ralls: > >>>>>>> On Jan 9, 2019, at 9:06 AM, Geert Janssens > >>>>>>> <geert.gnuc...@kobaltwit.be> > >>>>>>> wrote:> > >>>>>>> > >>>>>>> Op woensdag 9 januari 2019 15:45:31 CET schreef John Ralls: > >>>>>>>>> On Jan 9, 2019, at 6:17 AM, Derek Atkins <de...@ihtfp.com> wrote: > >>>>>>>>> > >>>>>>>>> HI, > >>>>>>>>> > >>>>>>>>> On Wed, January 9, 2019 9:12 am, Geert Janssens wrote: > >>>>>>>>>> I like the idea of caching the system locale for future use. Too > >>>>>>>>>> bad > >>>>>>>>>> the > >>>>>>>>>> std::locale is working so poorly on Windows :( > >>>>>>>>>> > >>>>>>>>>> Op zondag 6 januari 2019 19:13:28 CET schreef John Ralls: > >>>>>>>>>>> +const std::locale& > >>>>>>>>>>> +gnc_get_locale() > >>>>>>>>>>> +{ > >>>>>>>>>>> + static std::locale cached; > >>>>>>>>>>> + static bool tried_already = false; > >>>>>>>>>> > >>>>>>>>>> If I understand it correctly using static variables makes the > >>>>>>>>>> function > >>>>>>>>>> unsuitable for multi-threading, right ? > >>>>>>>>> > >>>>>>>>> Not necessarily. There is a race condition on first-use, but > >>>>>>>>> beyond > >>>>>>>>> that > >>>>>>>>> I don't see a MT issue here. The data is read-only, right? > >>>>>>>>> Multiple > >>>>>>>>> threads could read from the same read-only data simultaneously, so > >>>>>>>>> that > >>>>>>>>> should be fine. > >>>>>>> > >>>>>>> It was this first-use race condition I was referring to. > >>>>>>> > >>>>>>> Particularly, for thread safety I think setting tried_already = true > >>>>>>> should > >>>>>>> happen at the very end of the function, after we're sure cached has > >>>>>>> a > >>>>>>> proper value. Otherwise a competing thread could try to get the > >>>>>>> uninitialized cached value if thread execution of the first thread > >>>>>>> is > >>>>>>> interrupted right after tried_already is set true. > >>>>>>> > >>>>>>>>> Static data is ont MT-unsafe if it's being changed on a per-call > >>>>>>>>> basis > >>>>>>>>> (e.g. a time_t -> string API returning a static string buffer). > >>>>>>>>> > >>>>>>>>>> Any idea how difficult would it be to fix that ? > >>>>>>>>> > >>>>>>>>> You could add a mutex around the initialization. That's all I > >>>>>>>>> think > >>>>>>>>> you > >>>>>>>>> would need here. > >>>>>>>>> > >>>>>>>>>> I know GnuCash is not thread-safe by a long shot and gtk itself > >>>>>>>>>> is > >>>>>>>>>> single > >>>>>>>>>> threaded so it doesn't matter that much. > >>>>>>>>>> > >>>>>>>>>> However I silently set a personal goal of changing that sometime. > >>>>>>>>>> The > >>>>>>>>>> C > >>>>>>>>>> code > >>>>>>>>>> is a lost cause IMO, but it might be worth to keep > >>>>>>>>>> multi-threading > >>>>>>>>>> in > >>>>>>>>>> mind > >>>>>>>>>> as > >>>>>>>>>> we gradually convert to c++. In my basic understanding of > >>>>>>>>>> threading > >>>>>>>>>> this > >>>>>>>>>> particular function should not be too hard to make tread safe. > >>>>>>>> > >>>>>>>> Right, and I decided against making this the first introduction of > >>>>>>>> mutex > >>>>>>>> into GnuCash. I think std::atomic > >>>>>>>> (https://en.cppreference.com/w/cpp/atomic/atomic > >>>>>>>> <https://en.cppreference.com/w/cpp/atomic/atomic>) is the correct > >>>>>>>> modern > >>>>>>>> C++ way for a simple case like this. > >>>>>>> > >>>>>>> I was hoping indeed modern C++ has some answer to this. This is my > >>>>>>> first > >>>>>>> foray into multi-threading by the way so I'm pretty green in this > >>>>>>> area > >>>>>>> and wishing to learn more about it. > >>>>>>> > >>>>>>> In this particular case would declaring the cached and tried_already > >>>>>>> as > >>>>>>> atomic be sufficient to make this thread safe ? > >>>>>>> > >>>>>>> It seems to me this would still allow multiple threads to go in and > >>>>>>> run > >>>>>>> the > >>>>>>> initialization code for setting cached. Given they all should end up > >>>>>>> setting cached to the same locale it's probably not dangerous to the > >>>>>>> application, only potentially running the same code multiple times > >>>>>>> where > >>>>>>> only once would be sufficient. > >>>>>> > >>>>>> My knowledge of concurrency is pretty dated. I had an operating > >>>>>> system > >>>>>> course 30 years ago that included a discussion of concurrency, and > >>>>>> some > >>>>>> parts of Glib are thread-safe, so I’ve bounced up against it doing > >>>>>> maintenance a few times. Just mutex and semaphore stuff. > >>>>>> > >>>>>> I haven’t even yet read the concurrency chapter in Josuttis, but my > >>>>>> understanding of std::atomic is that a std::atomic variable magically > >>>>>> eliminates races and has certain operations (construction and > >>>>>> operator= > >>>>>> among them) that are guaranteed to be, well, atomic: The compiler > >>>>>> will > >>>>>> always create a contiguous uninterruptible sequence of machine > >>>>>> instructions > >>>>>> for atomic operations. There are also ways to order execution of some > >>>>>> functions with std::memory_model that can make locks (i.e. mutexes > >>>>>> and > >>>>>> semaphores) unnecessary, and the compiler is able to figure out when > >>>>>> that’s > >>>>>> true and when it isn’t and to take care of the locking without the > >>>>>> programmer needing to explicitly code it. > >>>>>> > >>>>>> It’s not something I think worth worrying about now. GnuCash is very > >>>>>> far > >>>>>> away from being multi-threaded. Besides, the natural home for the > >>>>>> instantiation of the cached C++ local is in main(), after all of the > >>>>>> environment options are parsed but before the GUI is loaded or the > >>>>>> first > >>>>>> session started. This isn’t the function to worry about concurrent > >>>>>> access. > >>>>> > >>>>> Fair enough. Though I'd think it should happen in libgnucash > >>>>> initialization at some point rather than in the gui code. But that's > >>>>> just > >>>>> a matter of sorting out all startup configuration we do at some point > >>>>> and > >>>>> estimate which part is needed for libgnucash and which part is only > >>>>> for > >>>>> the gui code. > >>>> > >>>> Um, I don’t think it’s right to consider main() part of “the GUI code”. > >>>> It’s the mandatory starting function for any program. > >>> > >>> Except when we want to interface with libgnucash from binding languages > >>> such as python or guile directly. At least in this context there's no > >>> main() that can explicitly set our libgnucash environmental requirements > >>> (like environment variables from the environment file or locale etc) > >>> that > >>> I know of. > >>> > >>> GUI was indeed a vague term to explain this. > >>> > >>> You could argue the init function itself could be exported so the > >>> bindings > >>> can call that as well, making the calling script the equivalent of the > >>> main() function. That would be a valid option. > >>> > >>>> There’s at present no > >>>> “libgnucash initialization” at runtime unless you mean > >>>> libgncmod_engine_init. We can make one, of course, or extend > >>>> libgncmod_engine_init, but that’s still going to be called from main() > >>>> (as > >>>> libgncmod_engine_init is now, via gnc_module_init) and should happen > >>>> before > >>>> a future thread-enabled GnuCash starts spawning threads. > >>> > >>> My message had many implicit assumptions. I'm aware there's no formal > >>> libgnucash initialization yet. This was thinking out loud about the > >>> future, a future libgnucash where we no longer depend on gnc-module > >>> (which is one of our long-term plans). > >>> > >>> At that point we'll need some form of libgnucash initalization and yes, > >>> libgncmod_engine_init would be a first potential candidate. > >>> > >>> A more radical idea (not fully analyzed by any means) could be to build > >>> up > >>> libgnucash as a class hierarchy in which the constructor of the > >>> top-level > >>> class does all the more general initialization (like configuring the > >>> environment and locale). > >>> In that case initialization would happen automatically at first > >>> instantiation of that class. That could happen in a main() function for > >>> a > >>> C++ program (like future GnuCash proper) or from a binding script. > >> > >> I don't think that writers of other programs would be happy to have > >> libgnucash doing its own localization determination separate from their > >> program, and especially not changing the environment behind their backs. > >> The portable way for localization is for the calling program to do > >> whatever > >> it wants to set the locale in the environment and for dependencies to > >> read > >> the environment if they need to. That's not to say that we couldn't move > >> our localization code to core-utils and expose it in the libgnucash API, > >> but it shouldn't happen automatically in case the application wants to do > >> something different. > > > > I don't think what I am trying to convey would do anything you worry > > about. > > Let's refine my thoughts a bit more then. > > > > I would like libgnucash to "just work" by default and not force calling > > code to do initialization unless they want to deviate from that default. > > That means reading necessary bits from the environment, which we only > > want to do once. So it makes sense to do so at the very first use of > > anything libgnucash in my opinion. > > I see similar things in for example libgtk. It behaves sensible by default > > but it can be tweaked by means of a settings.ini file or some environment > > variables. > > > > GnuCash as is currently stands already depends on environment in several > > ways: locale is one, paths to dbi drivers is another, as are load paths > > for guile. Most of these will also be required to make libgnucash > > function properly when we are ready to split it off. So I think it would > > be useful if libgnucash could manage a default configuration for these > > things with an exposed interface to allow callers to override these (be > > it via a configuration file, environment variables, direct function > > calls,... to be detailed). > > > > So the idea is that a properly installed libgnucash in a well defined > > environment is able to initialize itself. If overrides are needed, calling > > programs should be able to provide those. That should make setting > > configuration of libgnucash optional in the best case and easy in the > > worst. > > > > Note my idea of implicitly initializing on first use probably implies any > > overrides should be set in some way *before* that first use. So either in > > a > > config file, via the environment or via optional parameters to the > > constructor. The latter is probably not very feasible. > > Gtk has gtk_init. You call it, set up your GUI, then call gtk_main to start > the event loop. libguile has several initialization functions to choose > from: > https://www.gnu.org/software/guile/manual/html_node/Initialization.html#Ini > tialization. No "initialize on first use" there. > > Consider that "initialize on first use" means that *every* exported function > has to have a clause or belong to a class whose constructor checks for and > initializes the global state. If we just need to do it for localizing the > C++ UI locale, that's what get_cpp_locale() does and we're done, except > that we'd have to make that first call thread safe. If we need to do much > more, a library init function is cleaner and simpler... and might be > cleaner and simpler than making get_cpp_locale thread safe. > > This is all for the rather distant future, though, so let's defer figuring > it out til then. We'll forget by then anything we decide now. ;-)
Yeah, we'll probably have a clearer picture then. Geert _______________________________________________ gnucash-devel mailing list gnucash-devel@gnucash.org https://lists.gnucash.org/mailman/listinfo/gnucash-devel