search

berkmancenter / namae

Namae (名前) parses personal names and splits them into their component parts.
160 stars 32 forks source link

"Almost" Thread safe Namae #23

Closed hackling closed 6 years ago

hackling commented 6 years ago

Fixes: https://github.com/berkmancenter/namae/issues/22

If you cherrypick f85e78cfb404c8401ceb7225fcde596458d63621 onto master, you'll see it fail.

This may or may not be thread safe if you are changing the ParserOptions during thread execution, but at least the Parser class itself won't be sharing state between threads.

inukshuk commented 6 years ago

Awesome, thanks!

What we could do, is leave the options in Parser, but put the default options into a class instance variable which is merged into options passed to the constructor. What do you think?

hackling commented 6 years ago

If I understand your suggestion correctly, changes to the Parsers' options won't be global, and will only be for an instance of a Parser, which could possibly break this gem for anyone currently using it.

From the code I looked at, it looked like the gem wanted every Parser to use the same options. I guess that's fine, but the Parser class stores a whole bunch of state while its working, and because it's a singleton, thread issues occur.

I made these changes to break none of the existing API, but still allow thread safety to some extent.

Anyone that upgrades this gem won't have any issues upgrading, because you still are able to set the global ParserOptions through the Parser class.

If you want to go down the route of making breaking API changes, which I would totally be on board for, there's a whole lot of changes I would make. It all depends on what we're optimizing for here, but I would consider making the parser a functional class, rather than an object, and get rid of the need for such things as it's reset method.

However, the issue of thread safety still remains, and I would look at fixing that before introducing a whole bunch of API changes.

All that being said though, I've almost reached the limit of time that I can afford to contribute to this. Tracking down this bug was a nightmare 😛

inukshuk commented 6 years ago

Exactly, we'd still have defaults in a class instance, but once a Parser is created, its options won't be shared. The Singleton approach is really from a whole different time and I have no problem breaking the API and bumping major.

That said, if you're running out of time, I'm happy to push this to Rubygems now and make the breaking changes later, when I get to it myself.

inukshuk commented 6 years ago

Fixed on master. Thanks!