Fix threadsafety issue for multithreaded apps #609

Merged
merged 1 commit into from Jun 1, 2012

Conversation

Projects
None yet
3 participants

We use JRuby, and encountered a problem where our servers would occasionally return all 500s after startup with this stack trace:

/!\ FAILSAFE /!\  2012-05-21 17:59:15 -0500
  Status: 500 Internal Server Error
  undefined method `key?' for #<ActiveRecord::ConnectionAdapters::ConnectionManagement:0x4e08ad5e>
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/activerecord-2.3.11/lib/active_record/connection_adapters/abstract/connection_pool.rb:365:in `call'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/rack-1.1.3/lib/rack/builder.rb:73:in `to_app'
    org/jruby/RubyArray.java:1615:in `each'
    org/jruby/RubyEnumerable.java:830:in `inject'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/rack-1.1.3/lib/rack/builder.rb:73:in `to_app'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/omniauth-1.0.2/lib/omniauth/builder.rb:42:in `call'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/actionpack-2.3.11/lib/action_controller/string_coercion.rb:25:in `call'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/rack-1.1.3/lib/rack/head.rb:9:in `call'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/rack-1.1.3/lib/rack/methodoverride.rb:24:in `call'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/actionpack-2.3.11/lib/action_controller/params_parser.rb:15:in `call'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/actionpack-2.3.11/lib/action_controller/session/cookie_store.rb:99:in `call'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/activesupport-2.3.11/lib/active_support/cache/strategy/local_cache.rb:36:in `call'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/actionpack-2.3.11/lib/action_controller/failsafe.rb:26:in `call'
    /var/www/sites/spiceworks/frontend.shared/bundle/jruby/1.9/gems/actionpack-2.3.11/lib/action_controller/dispatcher.rb:106:in `call'

This pull requests appends @app to the @ins collection right after the initialization block is executed so that it's guaranteed to only be added once. This is way cheaper than adding synchronization to Omniauth::Builder#call, and still has the same effect as the old code without the race condition.

Contributor

sferik commented May 31, 2012

Looks good to me. @mbleigh what do you think?

Contributor

mbleigh commented May 31, 2012

Have you verified that this works both with Rack 1.4 and Rack 1.3 and prior?

Contributor

sferik commented May 31, 2012

Good question.

@mbleigh, you should talk to the @travis-ci guys about enabling @travisbot for pull requests for this repo. If that was enabled, we'd already know the answer to your question. 😉

@mbleigh should be a no-op for 1.4. I've verified the fix on rack 1.1.3 and I've confirmed that the initializer for Rack::Builder is unchanged from 1.1 to 1.3.6.

Contributor

sferik commented Jun 1, 2012

That should be good.

sferik added a commit that referenced this pull request Jun 1, 2012

Merge pull request #609 from spiceworks/threadsafety_fix
Fix threadsafety issue for multithreaded apps

@sferik sferik merged commit c2ceb5f into omniauth:master Jun 1, 2012

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