Fix multi threading issue

[ismasan]

What

This PR pre-loads Rails' URL helpers into a module on startup time, to prevent multi-threading issues (ex. Puma) documented here: puma/puma#647

Context

I encountered sporadic issues when running this as part of a production app.

After some digging, everything points to the issue linked to above. The solution is to pre-load Rails' route helpers into a module instead of accessing them in run time.

This has the side benefit of improving performance (see below).

Installation

Unfortunately, this change means that Zooplankton needs to be required after the Rails application and routes have been defined. So you need this in your Gemfile:

gem 'zooplankton', require: false

And then require the gem in an initializer

# config/initializers/zooplankton.rb
require 'zooplankton'

Performance

Calling Rails.application.routes.url_helpers directly seems to create a new module every time, which is slow. The solution is to include Rails.application.routes.url_helpers once at startup time into our own module, and use that module in the library.

I run a quick benchmark before and after this change.

# before
#       user     system      total        real
#   0.250000   0.000000   0.250000 (  0.250181)

# after
#       user     system      total        real
#   0.110000   0.000000   0.110000 (  0.114176)

So there seems to be a performance gain.

This is the benchmark script used (Ruby 2.3.1 on a MacBook Pro 3 GHz Intel Core i7, 16 GB 1600 MHz DDR3):

require 'bundler/setup'
require 'rails'
require 'benchmark'

module Plankton
  class Application < Rails::Application
  end
end

Plankton::Application.routes.tap do |routes|
  routes.default_url_options[:host] = 'http://example.com'
  routes.draw do
    root 'root#index'
    get '/post/:slug', to: 'posts#show', as: :post
    get '/post/:slug/comment/:comment_id', to: 'commendts#show', as: :comment
  end
end

require_relative './lib/zooplankton'

Benchmark.bmbm do |x|
  x.report do
    1.upto(1000) do
      Zooplankton.path_template_for(:post)
      Zooplankton.path_template_for(:comment, slug: 'foo')
    end
  end
end

[benhamill]

Hi! Thanks for this... I have to admit: I'm not really maintaining this gem much these days. I quickly read through the thread you linked and followed through to Evan's repro steps and the bug he files against MRI. It looks like the thread issue got fixed there in 2.3? What version of Ruby are you using?

I noticed that TravisCI is mad, too... I bet we should update some test dependencies and also push up to the latest 2 point versions of Rails/Ruby? Threading issues are hard to test, but is there some way we could write a test that shows the thread problem? Or do you think the performance aspect is enough to warrant changes, regardless of whether the threading thing is still a problem in newer versions of MRI? I ask because I really dislike the interface of having to require: false and then add an initializer. However, might be that's a necessary evil.

What do you think about all this stuff?

[ismasan]

Hi. Yeah I attempted to test it by spinning up multiple threads and using Zooplankton within them, to no avail. I deployed my branch to the production environment where this issue was surfacing (intermittently) and it seems to have stopped now.

I agree re. the require: false. I think potentially it could hook into Rails' lifecycle hooks so it knows to activate itself only after the app and routes have been loaded, but I'm not sure I'll have time to investigate that in the next few weeks unfortunately.

[benhamill]

@ismasan What versions of Ruby and Rails are you using in production?

[ismasan]

I was on Rails 4.2.5 and Ruby 2.1.5 when I submitted this PR. Now I'm on 4.2.9 and 2.3.1 for the same app, so the threading issue might have gone away (it definitely went away even before the upgrade, after I started using this branch of Zooplankton).

Still, it looks like the performance improvement alone would be nice to have, if I find the time to not make that require: false necessary.

[ismasan]

Ok I fixed one of the Travis errors by updating rspec. One more to go, the by now classic An error occurred while installing nokogiri

[ismasan]

I've added a railtie that makes sure to load everything up once the Rails app is fully initialized.

This means that the require: false is no longer needed.

Tests pass by manually calling the new Zooplankton.setup! (called by Rails in a full app). I tried it with a real app and it also works. However, I'm not sure how to test the Rails initialization process here. Apparently I could use this gem but that seems like a little too much boilerplate.