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

[#164] Make to_s in paginator threadsafe #374

Merged
merged 1 commit into from May 2, 2013

Conversation

Projects
None yet
7 participants
@bf4
Copy link
Contributor

bf4 commented Mar 29, 2013

Per #164 and #214 modifies the Paginator to_s method to be threadsafe without creating the overhead of a new thread and without changing the functionality of the code. Tested to work on ruby 1.8 and 1.9 for cruby, jruby, and rubinius.

Changes made:

  • Make explicit what the paginator to_s is doing
  • Add a flag to the subscriber when to suppress logging. No methods are undefined; thus, an undefined method cannot be called.

Notes:

  • In a high-load environment, may not suppress logging consistently as desired
  • At this point, threading is not necessary

Essential references I read on threading, concurrency, :

H/T @mperham @merbist @wycats @runpaint @headius @evanphx @brixen @jstorimer

Testing notes:

For JRuby, I had to enable c-extensions ( for sqlite3. I couldn't get the jdbc driver working )

I also had to have mongod running

Here are the gems I installed

gem install sqlite3 # 1.3.7 (requires c-extension support in JRuby)
gem install rails -v 3.2.3
gem install rails -v 3.0.3 # cruby 1.8 also requires rack and tzinfo installed
gem install rspec-rails # 2.13.0, rspec 2.13.1
gem install rr # 1.0.4
gem install datamapper # 1.2.0
gem install dm-rails # 1.2.1
gem install dm-sqlite-adapter # 1.2.0
gem install mongoid # 3.1.12 (not 1.8-compatible)
gem install mongoid -v 2.4.12
gem install mongo_mapper # 0.12.0
gem install sinatra sinatra-contrib # 1.3.2
gem install padrino-helpers # 0.10.7
[#164] Make to_s in paginator threadsafe
Make explicit what the paginator to_s is doing
In a high-load environment, may not
repress logging consistently as desired
@jc00ke

This comment has been minimized.

Copy link
Contributor

jc00ke commented Mar 29, 2013

I would love to see this merged (if it's correct) as we see errors in production because this isn't threadsafe.

@bf4

This comment has been minimized.

Copy link
Contributor

bf4 commented Mar 29, 2013

@jc00ke I believe this solution (which we've tested on our app via loader.io) is the smallest possible change without significantly changing the functionality.

I was unable to make it threadsafe by using threading and mutexes, etc.

The only other solution would be to change if subscriber to if subscriber && defined?(RUBY_ENGINE) which would work, but never suppress the logging in rbx or jruby. This is essentially what the hack initializer in #164 does

@brixen

This comment has been minimized.

Copy link

brixen commented Apr 2, 2013

@bf4 I didn't have time to read the whole thread but instead of doing stuff like if defined?(RUBY_ENGINE), consider using http://rubygems.org/gems/redcard

@bf4

This comment has been minimized.

Copy link
Contributor

bf4 commented Apr 2, 2013

@brixen right now I only wanted to check that it's not cruby, which having RUBY_ENGINE defined is sufficient for, and appears to be at the core of https://github.com/brixen/redcard/blob/master/lib/redcard.rb as well, though I see the advantage of encapsulating the type-check.. If this request gets accepted, perhaps we'll include that. Any reason you didn't use RbConfig::CONFIG['RUBY_INSTALL_VERSION']?

@brixen

This comment has been minimized.

Copy link

brixen commented Apr 2, 2013

@bf4 RUBY_ENGINE is only not defined in MRI 1.8.

@bf4

This comment has been minimized.

Copy link
Contributor

bf4 commented Apr 5, 2013

@brixen my bad re: where RUBY_ENGINE is defined. Fortunately, that detail does not affect this pull request.

As and aside, in this pull request I tried not to change anything significant about the rendering hack in the original code or the need for it. It would be better to find a cleaner way to achieve the same goal. e.g. In a twitter discussion Charles Nutter wrote "Would be great to eliminate the singleton…bad for perf."

@PlugIN73

This comment has been minimized.

Copy link

PlugIN73 commented Apr 17, 2013

It's very need for me! Accept please!

@kgx

This comment has been minimized.

Copy link

kgx commented Apr 29, 2013

As the 'newer and better' pagination plugin, Kaminari should be made threadsafe ASAP. Please accept or provide an alternative solution!!

@bf4

This comment has been minimized.

Copy link
Contributor

bf4 commented May 1, 2013

I've sent emails to the maintainers. I'm not sure what else I can do.

amatsuda added a commit that referenced this pull request May 2, 2013

Merge pull request #374 from bf4/164-threadsafe-paginator
[#164] Make to_s in paginator threadsafe

@amatsuda amatsuda merged commit 774eac2 into kaminari:master May 2, 2013

@jc00ke

This comment has been minimized.

Copy link
Contributor

jc00ke commented May 3, 2013

Thank you!

@matpowel

This comment has been minimized.

Copy link

matpowel commented May 7, 2013

Hrm, this worked fine for us.. can someone tell me why this wouldn't be a more simple fix which also works? It simply locks other threads out while it does the dirty class hack add/remove method thing, that way there's no race condition.

agworld@b84164f

@bf4

This comment has been minimized.

Copy link
Contributor

bf4 commented May 7, 2013

@matpowel I didn't try the specific code in that commit, but I did try variations of it (using a Mutex) that did not prevent errors. In any case, regarding simplicity, I'd argue that resorting to synchronizing code in order to dynamically add and remove methods to a logging subscriber is more complex than just adding a toggle to the subscriber to temporarily turn off logging. There's still room for code/performance improvements :)

@matpowel

This comment has been minimized.

Copy link

matpowel commented May 7, 2013

@bf4 the problem to me is that I don't think the new fix is actually threadsafe, we are running our mutex version in production and have tested with very high concurrency, the symptom we had before our mutex fix was exceptions because of race conditions where the method was being defined and un-defined.. the fix in this PR makes the race condition less likely but still a problem I think?

Take this code..

unless defined?(render_partial_with_logging)
  alias_method :render_partial_with_logging, :render_partial

Thread 1: render_partial_with_logging is not defined
Thread 2: render_partial_with_logging is not defined
Thread 1: alias_method
Thread 2: oops!

That's off the top of my head, I'm assuming trying to re-define the same alias will throw an exception but if not there are still many other problems with two threads trying to add the same method at the same time. The basic point is that here the class definition is a process singleton (ie shared amongst threads) and is being updated in multiple threads simultaneously which is bad, I don't think you can get around needing to use a mutex except to find a way to switch logging off using instance variables or thread-safe class variables only.

I could be missing something though :/

@bf4

This comment has been minimized.

Copy link
Contributor

bf4 commented May 8, 2013

You make a good point. However, since implementing this code, we have not
seen failures under very high load (JRuby 1.9 mode, Passenger). So, even
if it is imperfect, it is much, much better. :)

On Tue, May 7, 2013 at 6:31 PM, Matt Powell notifications@github.comwrote:

@bf4 https://github.com/bf4 the problem to me is that I don't think the
new fix is actually threadsafe, we are running our mutex version in
production and have tested with very high concurrency, the symptom we had
before our mutex fix was exceptions because of race conditions where the
method was being defined and un-defined.. the fix in this PR makes the race
condition less likely but still a problem I think?

Take this code..

unless defined?(render_partial_with_logging)
alias_method :render_partial_with_logging, :render_partial

Thread 1: render_partial_with_logging is not defined
Thread 2: render_partial_with_logging is not defined
Thread 1: alias_method
Thread 2: oops!

That's off the top of my head, I'm assuming trying to re-define the same
alias will throw an exception but if not there are still many other
problems with two threads trying to add the same method at the same time.
The basic point is that here the class definition is a process singleton
(ie shared amongst threads) and is being updated in multiple threads
simultaneously which is bad, I don't think you can get around needing to
use a mutex except to find a way to switch logging off using instance
variables or thread-safe class variables only.

I could be missing something though :/


Reply to this email directly or view it on GitHubhttps://github.com//pull/374#issuecomment-17577293
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment