After making the changes to use the alternative thread-safe Match function, the next failure is with obLocale which is another global. Commenting out "free (d->old_locale_string);" semi-works around it for now (with a memory leak) but it can still segfault elsewhere in the locale code.
Does anyone know exactly what problem the locale stuff is fixing? And a follow-on, is there another way to fix it? I have a test program at https://gist.github.com/baoilleach/1a7e517798804bdd96b3e9b4927092ce that reads and writes SMILES strings on multiple threads (with a Windows-specific API). After all of the changes on the pull request, it can now take quite a while before segfaulting (mostly in locale, but worryingly once in NewAtom). Progress of sorts, I guess :-) I think I'm done for the moment with this. - Noel On 19 February 2017 at 10:47, Noel O'Boyle <baoille...@gmail.com> wrote: > I've just realised that David Koes has already fixed this, by > providing just such an interface, a Match function where you pass in > the vector for storing the matches. I owe you a pint or two, David. > > Note to self: as part of the API cleanup, we should remove the other > functions. > > - Noel > > On 19 February 2017 at 10:21, Noel O'Boyle <baoille...@gmail.com> wrote: >> I've made progress, but my current problem is that it's not possible >> to match the same SmartsPattern on different threads (as required, for >> example, by the typers, where sets of smarts patterns are initialised >> once and then reused). The matchers store the results as part of the >> pattern instead of returning it directly, so two simultaneous matches >> against the same pattern will confuse things. I'm not sure of the >> exact details of a solution until I study the existing code, but it >> will involve separating the parsed smarts pattern data structure from >> any match results and I'm guessing this will need an API change. >> >> - Noel >> >> On 12 February 2017 at 20:18, Noel O'Boyle <baoille...@gmail.com> wrote: >>> ...googling through the mailing list, I see that David Lonie mentions >>> that OBErrorLog has problems, and Tim that all formats are singletons. >>> I need to think some more about this latter. >>> >>> - Noel >>> >>> On 12 February 2017 at 16:45, Noel O'Boyle <baoille...@gmail.com> wrote: >>>> Classes or variables with a global instance are susceptible to >>>> multithreading problems. It turns out that these are listed in the >>>> Python bindings. >>>> >>>> Classes: OBElementTable, OBTypeTable, OBIsotopeTable, OBAromaticTyper, >>>> OBAtomTyper, OBChainsParser, OBResidueData, OBPhModel, OBErrorLog >>>> Variables: Residue, ElemDesc, ResNo, ElemNo (these all from the bottom >>>> of residue.h) >>>> >>>> The following don't appear to be a problem: >>>> OBElementTable, OBTypeTable, OBIsotopeTable >>>> >>>> On my branch (https://github.com/baoilleach/openbabel/tree/fixglobalstate) >>>> I think I've fixed the following: >>>> OBAromaticTyper, OBAtomTyper, OBPhModel >>>> >>>> Roger didn't anticipate multi-processor architectures and it looks >>>> like OBChainsParser and OBResidueData as well as those variables >>>> listed above do have a problem. It should be possible to fix, but >>>> perceiving residues (or reading PDB and also I guess MOL2) will cause >>>> problems right now. >>>> >>>> I haven't really looked at OBErrorLog, and my time is up right now. >>>> Have I missed any classes that you know of? >>>> >>>> David, do you have any test code that reads an SDF multithreadedly, >>>> that could be used to force segfaults? >>>> >>>> Regards, >>>> - Noel >>>> >>>> On 11 February 2017 at 21:08, Noel O'Boyle <baoille...@gmail.com> wrote: >>>>> Here's of what I was thinking: >>>>> https://github.com/baoilleach/openbabel/commit/697b92180f9f61f738ef3e5c04d30340b9f8ee8e >>>>> >>>>> If it looks okay, I can make a similar change to other classes that >>>>> have the same problem (assuming one size fits all). >>>>> >>>>> - Noel >>>>> >>>>> >>>>> On 8 February 2017 at 09:32, Noel O'Boyle <baoille...@gmail.com> wrote: >>>>>> There's no memory overhead on the molecule class - it's unchanged. Let >>>>>> me make the change on a branch for one of the classes, and we can chew >>>>>> it over. >>>>>> >>>>>> - Noel >>>>>> >>>>>> On 7 February 2017 at 13:35, Koes, David <dk...@pitt.edu> wrote: >>>>>>> I think this is most definitely the way to go if you can tolerate the >>>>>>> API changes. The main drawback is potentially each molecule class >>>>>>> taking up more memory. I would welcome any changes that push openbabel >>>>>>> closer to thread safety.. >>>>>>> >>>>>>> David Koes >>>>>>> >>>>>>> Assistant Professor >>>>>>> Computational & Systems Biology >>>>>>> University of Pittsburgh >>>>>>> >>>>>>> >>>>>>>> On Feb 7, 2017, at 5:20 AM, Noel O'Boyle <baoille...@gmail.com> wrote: >>>>>>>> >>>>>>>> Hi there, >>>>>>>> >>>>>>>> Wouldn't a better solution to the race conditions that David Koes is >>>>>>>> experiencing with global state be to remove the global state? For the >>>>>>>> cases he mentioned, e.g. the OBAromTyper, the global state relating to >>>>>>>> a single molecule can easily be moved to a OBAromTyperPrivate class >>>>>>>> instantiated by a TypeThisMolecule() function in the global class. >>>>>>>> >>>>>>>> This is an API breakage, but only because these internal >>>>>>>> implementation functions were exposed. I think the time might be ripe >>>>>>>> for a couple of API cleanups, not for the sake of it, but where they >>>>>>>> limit or affect the toolkit's usage. >>>>>>>> >>>>>>>> Regards, >>>>>>>> - Noel >>>>>>> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
