Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NoMethodError in multithreading [issue-109] #110

Closed
wants to merge 1 commit into from

Conversation

myxaluch
Copy link
Contributor

@myxaluch myxaluch commented Nov 24, 2016

In connection with #109

@@ -63,19 +63,24 @@ def edit_emoji(emoji)

# Public: Find an emoji by its aliased name. Return nil if missing.
def find_by_alias(name)
names_index[name]
@mutex.synchronize do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you instead add the mutex guard around the code in all method? That way we can have the synchronize call only in one place.

Copy link
Contributor Author

@myxaluch myxaluch Nov 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mislav check all unless defined @all is in each methods names_index/unicode_index, and if we add mutex to allmethod only, we will not avoid problem, because, e.g. in first thread, in all, @all will be defined, but @names_index and @unicodes_index haven't yet, and second thread catch the NoMethodError.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. In that case, can we simply call all from names_index/unicode_index instead of having the all unless defined @all expression?

Alternatively to this whole PR, do you think it would be worth it to populate all emoji in memory as soon as this library loads? Then there would be no multithreading issues, and threads wouldn't be slowed down by the fact they need to be synchronized around Emoji.find_by_* calls.

Copy link
Contributor Author

@myxaluch myxaluch Nov 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mislav If we remove this check and add mutex to all only, parse_data_file crash with deadlock error, because it's use all method inside yourself - self.create(nil) do |emoji|. I can re-write create method, something like this, it's allow to use mutex only one time :

def create(name)
    emoji = Emoji::Character.new(name)
    if defined? @all
      @all << edit_emoji(emoji) { yield emoji if block_given? }
    else
      self.all << edit_emoji(emoji) { yield emoji if block_given? }
    end
    emoji
  end

About populate all emoji to memory - we think it's a great idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mislav Hello again!
What about your sentence to load all emoji as soon as library loads? Could I take it on myself or you've already started work on it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myxaluch Yes, please open an alternative PR with the approach of preloading all emoji at load. I think that's a much safer and simpler approach, and would make multithreading faster since no mutex will be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mislav OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants