ConnectionPool.checkout needs to be restructured to take account of ruby's "non-blocking" strategy for mutex ConditionVariables #5422

wants to merge 482 commits into

sorry @tenderlove, you're going to hate me for this one, but at this point I've done some due dillegence and am reasonably confident I've found an actual issue -- which does affect my app -- having to do with some deep stuff regarding how ruby Monitor ConditionVariable's work. It requires some restructuring and slight uglification of the ConnectionPool.checkout method.

This is a tricky one. I have a test, but due to the race condition nature of the bug, it only sometimes fails, so I didn't include it it in the pull commit not sure if that was appropriate, but it's included below.

The problem occurs when you have a lot of threads (more than connection pool size) all wanting to checkout connections for very very short periods of time.

Conceptually, this ought to be accommodated no problem. Each thread only needs the connection for a very short period of time, orders of magnitude less than the timeout -- even if there's more of them then there are connections, they should each be able to take a turn, waiting when neccesary and all finishing long before timeout no problem.

In fact, sometimes (race condition, so it appears in some runs and not others), some of these threads will end up raising a ConnectionTimeoutError --- long before the timeout value is reached. They appear to be giving up and not getting a connection in miliseconds, long before the 5 second time out.

Why? As far as I can tell, here's the deal.

  • threadA wants a connection, but the pool is full, so it blocks on @queue.wait(@timeout)

  • shortly thereafter, threadB checks in a connection, and does a @queue.signal

  • this does wake up threadA, however threadA does not immediately get access to the mutex to be in it's 'synchronization' block. Some other thread slips into 'synchronization' first, checks out the newly avail conenction, and then at a later point threadA gets access to the mutex 'synchronization' block
    ** It finds there are no connections available, assumes this means that it only woke up from @queue.wait because of a timeout, and throws the ConnectionTimeoutError . But the 5 seconds hadn't elapsed, hardly any time at all elapsed, it got woken up because there was a checkin, another thread just stole it first.

At first this seemed to be like it must be a bug in ruby Monitor/ConditionVariable itself, or else ConnectionPool somehow not synchronizing in enough places, or something.

However, reading up on how Monitors and ConditionVariables work, this seems like one accepted model for the way they work -- and appears to be how ruby works. The wikipedia article on Monitors refers to these two models as "blocking condition variables" and "non-blocking condition variables"

This article on Java refers to the two categories instead as "Signal & Continue" (non-blocking) and "Signal & Wait" (blocking), and says Java uses the "Signal & Continue" method:

Signal & Continue (SC) : The process who signal keep the mutual exclusion and the signaled will be awaken but need to acquire the mutual exclusion before going.

That appears to be what ruby is doing too, although Monitor rdoc doesn't specify, and I wasn't able to understand the code enough to be sure -- that matches observed behavior here. So, the process which signals keeps the mutex, the signaled awakens but just gets added to the queue to aquire the mutex to operate in it's 'synchronized' block, and other threads may in front of it. Yep, that's happening.

Here's a test case which on my machine fails in about 80% of runs. Tested with both MRI 1.9.3-p0 and jruby 1.6.7 in 1.8.7 mode. That jruby has the same race condition is not surprising based on info that Java also uses the non-blocking/Signal-and-continue approach.

 def test_wait_on_connection
    main_connections = []

    # Fill er up
    (pool.spec.config[:pool] || 5).times do
      main_connections << pool.checkout

    threads = []
    10.times do
      threads << do            
        pool.with_connection do |conn|
          assert conn
          sleep 0.01

    pool.checkin main_connections.shift

    assert_nothing_raised do
      threads.each {|t| t.join}

    main_connections.each {|c| pool.checkin c}


Without the included pull request, test fails due to race condition approximately 80% of runs on my machine, may differ on yours. (adding more runs to the 10 times may make it more likely to fail) Note that if you run just this test by itself, on failure run, it completes with failure in way less than the 5 second timeout, even though it reports a failure due to ConnectionTimeoutError "could not get connection in 5 seconds."

With the included pull request, I was not able to get the test to fail in a bunch of runs -- and additionally my much more complex app no longer exhibited the problematic behavior I was seeing, where it raised ConnectionTimeoutError's when I didn't see any reason why, unless given a larger connection pool than seemed like should be neccesary.

The restructure in the pull request changes the order of operations in the loop in #checkout, but it also has to do some ugly things with keeping track itself of how long it slept during the @queue.wait. I couldn't think of any other way for it to determine whether the timeout was really up, in cases where it returned from @queue.wait with a connection still unavailable. It may execute multiple @queue.waits in successive loop iterations. The total of all of them should be no more than approximately the timeout value, as it keeps a running count of waiting time. This is neccessarily not exactly the @timeout value because there's really no way to get that exact measure, but it'll be pretty close, and should be good enough for what @timeout is meant to accomplish.

Not sure if there's an existing test that asserts that when a thread really can't get a connection it does raise ConnectionTimeout, and in about 5 seconds -- but I verified myself this was still true post-commit. (such a test would take 5 seconds to complete, which would be annoying, unless perhaps using a pool with a much smaller timeout).

Phew. Hope I explained this clearly using no more words than needed, although it took a lot of words.

jonleighton and others added some commits Jan 20, 2012
@jonleighton jonleighton Fix another race condition.
From 2c667f6.

Thanks @pwnall for the heads-up.


Jonathan Roes document `:raise` option support for several helpers [ci skip] c701358
@kennyj kennyj Fix GH #4580. Rails 3.2: uninitialized constant ActiveSupport::Tagged…
@josevalim josevalim Merge pull request #4582 from kennyj/fix_4580
Fix  GH #4580. Rails 3.2: uninitialized constant ActiveSupport::TaggedLogging::ERROR
@waseem waseem Remove deprecation warning from console output when running
activesupport tests.
@josevalim josevalim Merge pull request #4586 from waseem/assert_with_no_deprecation_warnings
Assert with no deprecation warnings activesupport tests.
@jviney jviney Fix `$rails_rake_task` global variable warning without replacing the …
…value of $rails_rake_task if it is already set.

Fixes #4591.

Signed-off-by: José Valim <>
@carlosantoniodasilva carlosantoniodasilva Add missing require to Array#wrap in generators action methods d246da1
@iHiD iHiD Allow to accept ranges. c6dcc35
@josevalim josevalim Merge pull request #4604 from ihid/3-2-stable
3-2-stable: Fixed regression - unable to use a range as choices for
@josevalim josevalim Support decimal{1,2} and decimal{1-2} and decimal{1.2} so it works fi…
…ne with bash, zsh, etc, closes #4602
@jviney jviney The deprecated ActiveSupport::Base64.decode64 method should call ::Ba…
…se64.decode64 not ::Base64.encode64
@vijaydev vijaydev Merge pull request #4611 from jviney/3-2-stable
Fix ActiveSupport::Base64.decode64
@vijaydev vijaydev test base64 encode and decode 912f144
@josevalim josevalim Merge pull request #4601 from carlosantoniodasilva/patch-1
Add missing require to Array#wrap in generators action methods
@tenderlove tenderlove Added custom regexps to ASTs that have literal nodes on either side of
symbol nodes.  Fixes #4585
@tenderlove tenderlove work against 1-0-stable until a new journey is released 1a56a76

Not sure if this is a work in progress but just in case, fyi, my app won't boot after this commit.

rails-1a56a761530e/actionpack/lib/action_dispatch/routing/route_set.rb:268:in `to_proc': undefined method `literal?' for #<Journey::Nodes::Slash:0x10be0f400 @memo=nil, @left="/"> (NoMethodError)
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/route_set.rb:268:in `find_all'
    from journey-1.0.0/lib/journey/visitors.rb:48:in `call'
    from journey-1.0.0/lib/journey/visitors.rb:48:in `visit'
    from journey-1.0.0/lib/journey/visitors.rb:15:in `binary'
    from journey-1.0.0/lib/journey/visitors.rb:18:in `visit_CAT'
    from journey-1.0.0/lib/journey/visitors.rb:11:in `send'
    from journey-1.0.0/lib/journey/visitors.rb:11:in `visit'
    from journey-1.0.0/lib/journey/visitors.rb:47:in `visit'
    from journey-1.0.0/lib/journey/visitors.rb:6:in `accept'
    from journey-1.0.0/lib/journey/nodes/node.rb:16:in `each'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/route_set.rb:381:in `find_all'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/route_set.rb:381:in `build_path'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/route_set.rb:355:in `add_route'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/mapper.rb:1304:in `add_route'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/mapper.rb:1282:in `decomposed_match'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/mapper.rb:1268:in `match'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/mapper.rb:1268:in `each'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/mapper.rb:1268:in `match'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/mapper.rb:411:in `mount'
    from rails-1a56a761530e/actionpack/lib/sprockets/bootstrap.rb:28:in `run'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/route_set.rb:272:in `instance_exec'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/route_set.rb:272:in `eval_block'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/route_set.rb:287:in `clear!'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/route_set.rb:287:in `each'
    from rails-1a56a761530e/actionpack/lib/action_dispatch/routing/route_set.rb:287:in `clear!'
    from rails-1a56a761530e/railties/lib/rails/application/routes_reloader.rb:35:in `clear!'
    from rails-1a56a761530e/railties/lib/rails/application/routes_reloader.rb:33:in `each'
    from rails-1a56a761530e/railties/lib/rails/application/routes_reloader.rb:33:in `clear!'
    from rails-1a56a761530e/railties/lib/rails/application/routes_reloader.rb:15:in `reload!'
    from rails-1a56a761530e/railties/lib/rails/application/routes_reloader.rb:26:in `updater'
    from rails-1a56a761530e/activesupport/lib/active_support/file_update_checker.rb:78:in `call'
    from rails-1a56a761530e/activesupport/lib/active_support/file_update_checker.rb:78:in `execute'
    from rails-1a56a761530e/railties/lib/rails/application/routes_reloader.rb:27:in `updater'
    from rails-1a56a761530e/railties/lib/rails/application/routes_reloader.rb:9:in `execute_if_updated'
    from rails-1a56a761530e/railties/lib/rails/application/finisher.rb:66
    from rails-1a56a761530e/railties/lib/rails/initializable.rb:30:in `instance_exec'
    from rails-1a56a761530e/railties/lib/rails/initializable.rb:30:in `run'
    from rails-1a56a761530e/railties/lib/rails/initializable.rb:55:in `run_initializers'
    from rails-1a56a761530e/railties/lib/rails/initializable.rb:54:in `each'
    from rails-1a56a761530e/railties/lib/rails/initializable.rb:54:in `run_initializers'
    from rails-1a56a761530e/railties/lib/rails/application.rb:136:in `initialize!'
Ruby on Rails member

@cgriego you have to use journey 1-0-stable branch or wait for a new release

Thanks @rafaelfranca, maybe I'm wrong but shouldn't the Gemfile generator reflect that if people generate with the --edge option?

Ruby on Rails member

I don't know. Maybe @tenderlove have an answer

kennyj and others added some commits Jan 21, 2012
@kennyj kennyj Fix GH #4344. A defined callback in extended module is called too.

@indrekj indrekj Add failing test for date_select
date_select does not work when day is discarded, include blank option is
enabled and struct date itself is nil.
@indrekj indrekj Fix date_select nil and blank and order case d92b5fe
@joevandyk joevandyk datetime_select should work with -/+ infinity dates 59a408e
@rafaelfranca rafaelfranca Fix date_select with discard_month and discard_year options
Closes #4553 and #4633
@tenderlove tenderlove Merge pull request #4514 from brainopia/update_timezone_offets
Update time zone offset information
@josevalim josevalim Merge pull request #4641 from rafaelfranca/date_select-fix-3-2
date_select fixes [3-2-stable]
@rafaelfranca rafaelfranca Remove unused variables to avoid warnings 3cd28e2
@josevalim josevalim Merge pull request #4643 from rafaelfranca/3-2-stable
Remove unused variables to avoid warnings
@tenderlove tenderlove Merge pull request #4639 from joevandyk/datetime-infinity-select-fix
datetime_select fix for dates of -infinity and +infinity
@pwim pwim allow requiring of 'active_model/naming'
Requiring 'active_model/naming' will raise an uninitialized constant
ActiveSupport::Deprecation exception because the module core extension
doesn't require 'active_support/deprecation'. This require cannot be
added to the core extension because of circular dependency issues.
@josevalim josevalim Merge pull request #4654 from pwim/explicit-deprecation-require
Explicit deprecation require
@pwim pwim Test for unicode path support
This is currently broken due to a bug in journey.

Is this a typo? What is wrong here? Should be ENCODE not DECODE.

Ruby on Rails member

@lzap look at the method name, this is self.decode64 method, therefore it should call decode

Ah it was wrong and now it is correct. Was coming from ApiDoc where it was still incorrect :-) Great. TY.

tenderlove and others added some commits Jan 25, 2012
@tenderlove tenderlove Merge pull request #4658 from pwim/unicode-paths
Test for unicode path support
@damln damln example bracket error fd9d394
@rohit rohit remove extra 'the' a267090
@carlosantoniodasilva carlosantoniodasilva Fix custom validation methods section in AR validations and callbacks…
… guide

The methods validate_on_create and validate_on_update are not available
anymore in Rails, this removes them from the guide and adds an example
on how to use validate with the :on option.
@carlosantoniodasilva carlosantoniodasilva Change ActiveRecord::Errors to ActiveModel::Errors in guides
Use ActiveModel::Errors in inflection example docs as well.

Also fixes wrong information and link to locale file related to
Errors#full_messages in I18n guide.
@carlosantoniodasilva carlosantoniodasilva Improve field error proc example in validations guide 79a0488
@carlosantoniodasilva carlosantoniodasilva Fix callbacks order for destroying an object in validations guide
Also add around save to creating/updating callbacks order, and fix save
example with no validation
@lunks lunks Duping log handler. fixes #4668 c9cd0eb
@vijaydev vijaydev Merge pull request #4673 from carlosantoniodasilva/validation-guides-…

Validation guides update 3 2
@tenderlove tenderlove Merge pull request #4675 from lunks/3-2-stable
Duping log handler on buffered logger silencer.
@lest lest call to_s on value passed to table_name=
Signed-off-by: José Valim <>
@brainopia brainopia Fix contributing guide to reflect preferred position on indentation 3a087b8
@fxn fxn disable automatic explain if there is no logger [closes #4671] 2483460
@fxn fxn registers 2483460 in the CHANGELOG 2809375
@rafaelfranca rafaelfranca Remove extra attributes from HABTM join tables in AR tests
HABTM Join tables should not have extra attributes

When extra attributes is needed in HABTM join tables is better to use
`has_many :through` association.

Fix #4653
@tenderlove tenderlove global variables may not be set depending on the match. fixes #4703 7ba3ecc
@tenderlove tenderlove Merge pull request #4696 from rafaelfranca/issue-4653
Remove extra attributes from HABTM join tables in AR tests
@tenderlove tenderlove bumping journey to 1.0.1 70cbb1c
@atambo atambo safe_constantize should handle wrong constant name NameErrors Fixes #…

Signed-off-by: José Valim <>
@josevalim josevalim Revert usage of safe constantize
Signed-off-by: José Valim <>
@fxn fxn CHANGELOG revision for v3.2.1 97e8d1d
@fxn fxn updating RAILS_VERSION 67b8fbc
@fxn fxn fixes whitespace in CHANGELOG entries 8e6ef37
@jonleighton jonleighton Merge pull request #4715 from pwim/find-create-multi-args
Fix regression from Rails 3.1
Tadas Tamošauskas check_box helper with :disabled => true generates disabled hidden fie…
…ld. fixes #1953
@dmathieu dmathieu don't set the hidden checkbox value if it's nil defb751
@josevalim josevalim Merge check box fixes from remote-tracking branch 'cantonio/checkbox-…
…hidden-backport' into 3-2-stable
@dhh dhh Revert "Fix expanding cache key for single element arrays"
This reverts commit abe915f.

This broke all existing keys and it's wrong anyway. The array is just there as a convenience option for building the string. It's intentional that [ "stuff"] and "stuff" generates the same key.
@dhh dhh Inline the prefix assignment so it doesnt look so daft f78cb55
@vijaydev vijaydev fix typo [ci skip] 766e12a
@fxn fxn query cache instrumentation should included the bindings in the paylo…
…ad [closes #4750]
@bsodmike bsodmike clarification to prevent confusing newbies; Passenger/Unicorn are app…
… servers, Apache/Nginx are the web servers along with thin a la Heroku etc.
@jonleighton jonleighton Merge pull request #4763 from kennyj/fix_4754
[MySQL] Fix GH #4754. Remove double-quote characters around PK when using sql_mode=ANSI_QUOTES
@jonleighton jonleighton Add workaround and deprecation if the inherited hook is not executed.…
… Closes #4757.
@jonleighton jonleighton Allow writing unknown attributes, but with a deprecation warning. Clo…
…ses #4583.
@lest lest fix assets test cfea673
@josevalim josevalim Merge pull request #4786 from lest/patch-2
fix assets test in 3-2-stable
@kennyj kennyj Fix GH #4760. A Block was not evaluated. 91700bf
@marten marten Fix use of Deprecation without requiring active_support/deprecation i…
…n ActiveSupport::Concern
@marten marten Fix use of Deprecation without requiring active_support/deprecation i…
…n ActiveSupport::Concern in Base64
@marten marten Fix use of Deprecation without requiring active_support/deprecation i…
…n message verifier
@marten marten Fix use of Deprecation without requiring active_support/deprecation i…
…n whiny nil
@josevalim josevalim Merge pull request #4790 from marten/fix-require-deprecation-in-activ…

Fix Deprecation usage in ActiveSupport when requiring only parts of AS
@jonleighton jonleighton Merge pull request #4783 from gregolsen/ids_reader_fix
ids_reader method fixed, test added to has_many association (for PostgreSQL)
@josevalim josevalim Merge pull request #4793 from kennyj/fix_4760
[3-2-stable] Fix GH #4760. A Block was not evaluated.
@tenderlove tenderlove Merge pull request #4779 from bsodmike/bsodmike-3-2-stable
Update comment in Gemfile re Unicorn (minor clarification)
@tenderlove tenderlove Merge pull request #4735 from arton/master
Re-launch assets:precompile task using original $0 if $0 is batch file so it works on Windows
@jonleighton jonleighton Improve deprecation message e1de540
@rafaelfranca rafaelfranca Fix broken tests added by 85c724d f532cd1
@spastorino spastorino Merge pull request #4811 from rafaelfranca/3-2-stable-fix
Fix broken build in 3-2-stable branch
@kennyj kennyj Fix GH #4749. Remove branch options, because edge is not 3-2-stable. c8c8439
@carlosantoniodasilva carlosantoniodasilva Generate strict validation error messages with attribute name
Backported from master.
@spastorino spastorino Merge pull request #4815 from kennyj/fix_4749
Fix GH #4749. Remove branch options, because edge is not 3-2-stable.
@josevalim josevalim Merge pull request #4822 from carlosantoniodasilva/strict-validation-3-2
Generate strict validation error messages with attribute name (3-2-stable)
@sikachu sikachu Fix plugin_new test failure from c8c8439
`plugin_new` generator doesn't generate `sass-rails` in the Gemfile, so
you can't check for that.
@josevalim josevalim Merge pull request #4827 from sikachu/3-2-stable-fix-plugin-new
Fix plugin_new test failure from c8c8439
@jonleighton jonleighton Merge pull request #4543 from jdelStrother/find_or_init
Don't instantiate two objects in collection proxy / find_or_instantiate_by
@tenderlove tenderlove Merge pull request #4809 from cfeist/feist-sqlite-binary-corruption
Fix for SQLite binary data corrupter (to master branch)
@densya203 densya203 Fix Issue #4819
'uninitialized constant ActiveRecord::Deprecation in Rails3.2.1'

Just a typo of 'ActiveSupport::...'
@tenderlove tenderlove Merge pull request #4868 from skult/3-2-stable
Fix Issue #4819
@sikachu sikachu Fix override API response bug in respond_with
Default responder was only using the given respond block when user
requested for HTML format, or JSON/XML format with valid resource. This
fix the responder so that it will use the given block regardless of the
validity of the resource. Note that in this case you'll have to check
for object's validity by yourself in the controller.

Fixes #4796
@fxn fxn let automatic EXPLAIN ignore CACHE notifications ee6e3c1
@josevalim josevalim Merge pull request #4879 from kennyj/fix_4873
Fix GH #4873. Allow swapping same class middleware.
@josevalim josevalim Merge pull request #4870 from sikachu/3-2-stable-responder-fix
Fix override API response bug in respond_with
@josevalim josevalim Clean up a bit default_response handling and cache format negotiation. 2bf2055
@kennyj kennyj GH #4883. Optional start_day argument for Time#all_week b037401
@spastorino spastorino Merge pull request #4890 from kennyj/improvement_4883
GH #4883. Optional start_day argument for Time#all_week
@josevalim josevalim Merge pull request #4908 from kennyj/fix_3864
Fix url_for method's behavior. GH #3684.
tenderlove and others added some commits Feb 22, 2012
@tenderlove tenderlove Merge pull request #5087 from pwnall/no_view_logger
Remove reference to config.action_view.logger from Rails configuration guide
@josevalim josevalim Avoid inspecting the whole route set, closes #1525 419040f
@tenderlove tenderlove Merge pull request #4834 from sskirby/fix_usage_of_psql_in_db_test_pr…

Fix usage of psql in db:test:prepare
@tenderlove tenderlove Merge pull request #4834 from sskirby/fix_usage_of_psql_in_db_test_pr…

Fix usage of psql in db:test:prepare
@arunagw arunagw Build fix for ruby187-p358 99fa8e5
@fxn fxn Merge pull request #5164 from arunagw/build_fix_ruby187-p358
Build fix ruby187 p358
@arunagw arunagw Checking headers in a better way. as doing here 0bfcd4f
@fxn fxn Merge pull request #5166 from arunagw/3-2-stable
3 2 stable
@arunagw arunagw assert => assert_equal a9b1397
@spastorino spastorino Merge pull request #5170 from arunagw/3-2-stable
assert => assert_equal
@noahhendrix noahhendrix Fixed typo in composed_of example with Money#<=>, was comparing amoun…
…t itself instead of other_money.amount
@glitterfang glitterfang Fix typo in match :to docs b229bc7
@RalphShnelvar RalphShnelvar Fixing Windows asset tag helper test failure
In asset_tag_helper_test.rb there is an assert on the number of bytes in a
concatenated file.  This test failed because Windows converts \n to \r\n as
the default for "w".  This is different than in *nix systems where there is
no conversion done.

THe test that failed was test_caching_stylesheet_link_tag_when_caching_on

Using bin mode fixes this behavior on windows and makes no change on the
*nix systems.
@pixeltrix pixeltrix Adding tests for non-optional glob parameters b8c4f2d
@rafaelfranca rafaelfranca Revert "No need to pass options which is never used"
Options is needed for some Rails extensions to determine when
referential integrity should be disabled

This reverts commit bcb466c.

Fixes #5052
@rafaelfranca rafaelfranca Add a new line after the textarea opening tag.
Closes #393
@tenderlove tenderlove Merge pull request #5179 from RalphShnelvar/Binary_mode_Window_bug
Binary mode window bug
@tenderlove tenderlove Merge pull request #5190 from rafaelfranca/fix-393-3-2-stable
[3-2-stable] Add a new line after the textarea opening tag.
@tenderlove tenderlove call binmode on the tempfile for Ruby 1.8 compatibility 72ae0b4
@kennyj kennyj Fix type_to_sql with text and limit on mysql/mysql2. Fix GH #3931. f1f2e8c
@tenderlove tenderlove Merge pull request #5206 from kennyj/fix_5173-32
[3-2-stable] Fix type_to_sql with text and limit on mysql/mysql2. Fix GH #3931
@josevalim josevalim Ensure [] respects the status of the buffer. 55ac1b9
@tenderlove tenderlove Merge branch '3-2-stable-security' into 3-2-2
* 3-2-stable-security:
  Ensure [] respects the status of the buffer.
  delete vulnerable AS::SafeBuffer#[]
  use AS::SafeBuffer#clone_empty for flushing the output_buffer
  add AS::SafeBuffer#clone_empty
  fix output safety issue with select options
@tenderlove tenderlove bumping to 3.2.2 01b470f
@tenderlove tenderlove Merge branch '3-2-2' into 3-2-stable
* 3-2-2:
  bumping to 3.2.2
  Ensure [] respects the status of the buffer.
  Merge pull request #4834 from sskirby/fix_usage_of_psql_in_db_test_prepare
  Merge pull request #5084 from johndouthat/patch-1
  updating RAILS_VERSION
  delete vulnerable AS::SafeBuffer#[]
  use AS::SafeBuffer#clone_empty for flushing the output_buffer
  add AS::SafeBuffer#clone_empty
  fix output safety issue with select options
@fxn fxn revert setting NOT NULL constraints in add_timestamps
Commit 3dbedd2 added NOT NULL constraints both to table
creation and modification. For creation the new default
makes sense, but the generic situation for changing a
table is that there exist records. Those records have
no creation or modification timestamps, and in the
general case you don't even know them, so when updating
a table these constraints are not going to work. See
a bug report for this use case in #3334.
@drogus drogus Fix #5238, rendered_format is not set when template is not rendered 1a71e84
@carlosantoniodasilva carlosantoniodasilva Stop SafeBuffer#clone_empty from issuing warnings
Logic in clone_empty method was dealing with old @dirty variable, which
has changed by @html_safe in this commit:

This was issuing a "not initialized variable" warning - related to:

The logic applied by this method is already handled by the [] override,
so there is no need to reset the variable here.
@spastorino spastorino Add CHANGELOG entry 8674823
@tenderlove tenderlove only log an error if there is a logger. fixes #5226

@spastorino spastorino Turn off verbose mode of rack-cache, we still have X-Rack-Cache to ch…
…eck that info

Closes #5245
@vijaydev vijaydev CSS fix for guides. Closing #5028 [ci skip]
In Ubuntu Chrome, in the last lines of code blocks, the underscore isn't
visible. Increasing the line height slightly seems to fix this. This
problem doesn't exist in Firefox even on Ubuntu. Too lazy to test in
any other OS-browser combo :)
@larskanis larskanis fix associations when using per class databases
would get ConnectionNotEstablished error because it always tried to use
ActiveRecord::Base's connection, even though it should be using the connection
of the model whose context we're operating in
@carlosantoniodasilva carlosantoniodasilva Only run binary type cast test with encode! on Ruby 1.9 864d755
@NZKoz NZKoz Whitelist all attribute assignment by default.
Change the default for newly generated applications to whitelist all attribute assignment.  Also update the generated model classes so users are reminded of the importance of attr_accessible.

Thanks! Guess the change in config/application.rb would be enough, but people tend to create a shitstorm instead.

Was this really intended for 3-2-stable?

Looks great! I like it!



Ruby on Rails member

@parndt it will be on only for new apps, so yes it's safe to do it on 3-2-stable


Nice one!

Once I started using attr_accessible consistently, it was handy to have a role that could access any attribute. sudo_attr_accessibility is the result I came up with


About time this was made default. It's just way too easy to forget about things with a permitted-by-default scheme.

@eval Considering that sudo is short for su (super-user) and do, wouldn't a more apt name for the access-all role be just su?

Ruby on Rails member

@eval you can specify the role, not sure when was it added, but probably 3.1/3.2

Guess we can thank @homakov for that.

@drogus Yeah but @eval's way of doing it gives access to all attributes, the roles only give the permissions set.

I think this is a great way to deal with the issue, as it doesn't breaks old apps. I really like the patch.

The downside is, that old apps still keep the vulnerability, maybe we could also add some warnings, similar to deprecation warnings like, "Your model blacklists your attrs and may be vulnerable to mass-assignment - consider using attr_accessible" (And a way to shut the warning of, for those who doesn't care :)

Old apps would keep this vulnerability no matter what they add if they don't update their Rails version.

Ruby on Rails member

@larzconwell it's dead simple to do something like that in previous rails versions, by doing for example ActiveRecord::Base.attr_accessible nil, the thing is that if you have a big app that does not whitelist attributes, it will all break if you turn it on. That's why there is no simple magical fix for it.

How would that code get into someone's Rails version that's already downloaded though? They'd have to re-install Rails, or update the version to get any new code. Right? Maybe I'm just not thinking it through all the way.

Thanks for fixing this @homakov !!!

Ruby on Rails member

@larzconwell this commit only changes things that are generated, so even if you upgrade, it will not change anything in your application. If your app is vulnerable, you need to use attr_accessible or secure your models in some other way, there is no simple fix for it as this is not regular security issue that can be easily fixed by framework. It was probably mistake to not make whitelisting default, but the tools to secure mass assignment was in place for years.

Oh yeah I knew that, just a slow moment haha. Sorry for the confusion!

Seems legit.


Since it really isn't a rails-framework issue, I think it is important to make developers aware of it. If you do regular rails upgrades in your apps (which you really should for security fixes), you should at least got some warnings on vulnerable models.

A walk through as to how to incorporate this into existing web applications will be nice.

@marcoemrich Definitely true, developers should use attr_accessible no matter what.

@why-el Well an existing application should already have attr_accessible in it, but un-comment config.active_record.whitelist_attributes = true into your config/application.rb

Older rails applications will still be vulnerable even when they upgrade their rails version. This commit only fixes the generator of the config file. Granted ofcourse that they were not handling their attribute protection right, like github was.

@larzconwell Done. Thanks.

@d-snp right, of course - that's why I would suggest to add a warning system, that informs the developers about their possible vulnerability and recommend switching to a whitelist concept

Rails could output a warning, for all models, that don't use attr_accessible

thanks @homakov!

a thousand crackers just shed a tear

Nice! I like it.

Finally! I've been advocating something like that (through an initializer) at my job and nobody would listen. I can't count the number of vulnerable web app I've seen out there!

Ah, the story has a happy ending!

@drogus yes, but once you define any role (say 'attr_accessible :name, :as => :possibly_malicious_user_input') i.e. specifically for mass-assignments in controllers, there's no way to ignore roles in other parts of your code (e.g. tests, working in console, whole lot's of non-controller code).

Ways to workaround are
1. use ':without_protection => true' (cumbersome),
2. explicitly (re)define all attributes to be attr_accessible for some other role, maybe 'default' (hard to maintain when adding attributes).

This gem helps with 2) in that you can define a role that has always access to all attributes. Maybe better: When doing 'sudo_attr_accessible_as :default' code can be role-unaware by default.

@dirk good point!

Ruby on Rails member

@eval ok, sorry, I just skimmed through the README and didn't get that difference

It still auto-adds the attr_accessible list which is nearly as bad as allowing mass assignment in the first place. At most it should add an informative comment, and the error message about being unable to mass assign attributes should be made better, possibly with a URL to the existing mass assignment Rails guide.

Ruby on Rails member

@eval write a custom TestSanitizer that doesn't remove anything from the attribute hash and configure it in your test environment or you can do it on a test by test basis:

class TestSanitizer < ActiveModel::MassAssignmentSecurity::Sanitizer
  def sanitize(attributes, authorizer)
    attributes # you may want to do something else here

class MyTestCase < ActiveSupport::TestCase
  def test_case
    without_attribute_protection do
      # your test code

  def without_attribute_protection
    ActiveRecord::Base.mass_assignment_sanitizer =
    ActiveRecord::Base.mass_assignment_sanitizer = :logger

@jyap808 No, that wasn't a proper troll, but your comment certainly is :)

@dhh posted this gist[0] earlier today and I'm wondering what everyone feels is the best way to handle this, I'm currently using attr_accessible but I do feel there's some merit to dhh's gist.


If nothing else, this breaks all 6+ years of prior documentation on generating models and applications. This seems inappropriate and unnecessary in a stable release, when the current behavior is not unreasonable and was so vociferously defended for years.

@allix I disagree, the current behavior is unreasonable and unacceptable since every application is insecure by default. The fact that Github could fall victim to a mass assignment vulnerability should be more than enough evidence that this change is necessary. In fact, I'd go even further and say that config.active_record.whitelist_attributes should be set to true by default (not just in the generated application.rb) and developers who don't want it should have to specifically disable it. Any documentation or app that is "broken" by that change was already broken anyway, the developers just didn't realize it, so this change would wake them up and make their apps more secure in the long run.

@stevenh512 Do we even know that GitHub was not using attr_accessible? They could have just opened up columns that are only accessible to admins to all users because of the bluntness of attr_accessible (somewhat remedied by the new role based feature).

Regardless, Rails can't protect developers against not paying attention when they apply untrusted form data directly to models - that is just an amateur mistake, and it seems most applications do just fine as their developers properly filter parameters before updating models. This is quite clearly the fault of GitHub developers, and it's unclear whether attr_accessible by default would have changed anything.

True, we don't really know that Github wasn't using attr_accessible, the only way to truly know the exact cause of their particular vulnerability is if they tell us.. but we do know that there are plenty of apps out there that are not using attr_accessbile because 1) it is not enforced by default, 2) there's a lot of bad documentation out there, and 3) beginner/intermediate developers see that it "just works" and don't think about the security implications (which goes back to 1). I think it's a lot easier to fix it in Rails and "break" those already broken applications than it would be to go through (as you said) 6+ years of documentation, fix it, and then hope everybody pays attention.

Fixing XSS vulnerability "broke" a lot of things too, but again, those things were already broken.

BTW, how many years of documentation were broken in moving from Rails 1 to 2, or 2 to 3, or even (NOT a "major version" upgrade) 3.0 to 3.1?

@pixeltrix looks nice - thanks!

Ruby on Rails member

Maybe I'm outdated, but I remember a Github conf saying that Github is still mostly under Rails 2.3.
So they do not use attr_accessible.

On another topic, I think this is not the role of the model layer to sanitize the user input.

Ruby on Rails member

@drogus: wow, why was I persuaded that it was a 3.0 feature... Sorry for this useless comment.

@stevenh512 regarding documentation, breaking documentation is fine for major or even minor releases. Not so much for a stable release. Anyhow, it's probably for the better.

@aliix regarding documentation, if that's the case, why not really fix this (instead of just for new apps) and call it 3.3.0?

And regarding apps, this is just my personal opinion.. but if I had an app that was vulnerable (I don't, becuase step 1 for me is to drop in an initializer that forces me to use attr_accessible.. but for the sake of argument, if I didn't know any better), I would much rather deal with "a Rails update broke my app" compared to "a vulnerability I wasn't aware of because I followed some bad docs caused my entire app to get owned and now my users are ready to hang me."

This is a good step, thanks for the fix!

I think there should be at least warnings, for older code, in the measure of possible, to raise awareness, by default. If not downright break those with no attr_accessible.

+1 on the fix, but the reason mass assignment exists in the first place is for convenience and DRYness of syntax. Sometimes it's still convenient to use update_attributes outside of a controller for 'unsafe' attributes and with a default whitelist you can't. What about something that mirrors #html_safe?:

foo.update_attributes({:a => 'bar'}.attr_safe)

@norv I agree, but I go a step further and say that a change to force mass-assignment security on all apps (not just new ones), with some reasonable way for a more advanced developer to disable it if they really needed to, wouldn't break a thing. Any app with those kinds of vulnerabilities was already broken the moment it was written and deployed and it's only a matter of time before someone exploits it.

@andyvb didn't see your comment while I was typing mine (we must have been commenting at the same time.. lol).. but the situation you describe is already handled by ActiveModel in Rails 3.1+.. that's what scoped attr_accessible whitelists are for.

@stevenh512 thanks for the info! Looks like :without_protection is what I'm looking for.

@andyvb I was thinking more of the scoped attr_accessible that Rails 3.1 gives us (attr_accessible :attr1, :attr2, :as => :admin), but yeah, :without_protection would also work and would probably be more backwards compatible. For 3.0 and earlier there's also a Railscast that teaches how to do something similar to 3.1's scoped attr_accessible.

What about *_ids methods?

Lot of people use attr_protected for keys and flags, simply because it is easier to blacklist a few fields than to withelist the rest.

How many of them ever used *_ids method to be aware of that? This is not even documented at

This SHOULD be protected by default in next release.

What about *_ids methods?

Lot of people use attr_protected for keys and flags, simply because it is easier to blacklist a few fields than to withelist the rest.

How many of them ever used *_ids method to be aware of that? This is not even documented at


rafaelfranca and others added some commits Mar 4, 2012
@rafaelfranca rafaelfranca Only add the whitelist_attributes option if ActiveRecord is present

@rafaelfranca rafaelfranca Now all the models need to explicitly declare the accessible attributes 2a6b7e5
@NZKoz NZKoz Merge pull request #5278 from rafaelfranca/fix-build-3-2
[3-2-stable] Fix build
@byroot byroot Fix #5069 - Protect foreign key from mass assignment throught associa…
…tion builder
@mariovisic mariovisic Failing test for mime responder respond_with using a block. 5b73a3a
@sikachu sikachu Always passing a respond block from to responder
We should let the responder to decide what to do with the given
overridden response block, and not short circuit it.

Fixes #5280
@josevalim josevalim Merge pull request #5299 from sikachu/3-2-stable-fix-responder
Always passing a respond block from to responder
@mikel mikel Increasing minimum version of mail due to security vulnerability foun…
…d in Mail 2.4.1 for sendmail or exim
@josevalim josevalim Use latest rack-cache. 9c56401
@spastorino spastorino Deprecate ActionController::SessionManagement 74fe7e1
@josevalim josevalim Just change the formats on first render, closes #5307, closes #5308. bcea8cd
@josevalim josevalim Set the rendered_format on respond_to. e7560bc
@carlosantoniodasilva carlosantoniodasilva Add test case for #5307 b35fd40
@josevalim josevalim Remove usage of deprecated module. 3775058
@josevalim josevalim Merge pull request #5316 from Jacobkg/master
Update ActiveRecord::AttributeMethods#attribute_present? to return false for empty strings
@kuahyeow kuahyeow Add tests to test that through associations are not readonly, and we …
…can update the records we retrive from the association
@carlosantoniodasilva carlosantoniodasilva Improve docs for attr_accessible|protected related to Hash#except|slice e63f04c
@Mik-die Mik-die typo 1f2224d
@swanandp swanandp Fixed a slightly misleading equivalent SQL code on the 3.2 query inte…
@bschaeffer bschaeffer Fix doc examples for has_and_belongs_to_many association 357e288
@mattreduce mattreduce Fix typo in asset pipeline guide 061f8ae
@caius caius Fix typo in isolated engine docs 22a8433
@coreyhaines coreyhaines Left off a : when specifying the :namespace option for a :controller
path segment
@vijaydev vijaydev In a nested resource route, the parent resource param is <resource_na…

This fix was made by @coreyhaines on docrails and merged in master.
Cleanly cherry picking into 3-2-stable wasn't possible.
@vijaydev vijaydev changelog updates [ci skip] 3bfd651
@vijaydev vijaydev update changelogs for gems without changes too [ci skip] 263d842
@tenderlove tenderlove make active_connection? return true only if there is an open connecti…
…on in use for the current thread. fixes #5330


curious if this will help any regarding brianmario/mysql2#66, brianmario/mysql2#209 or brianmario/mysql2#213

Ruby on Rails member

@brianmario I don't think so... I've seen those errors before, but I can't remember exactly the problem. IIRC, it happens if you have a low traffic app where the connection timeout is not long enough, but I can't remember.

@jeremy do you recall issues like these? I seem to remember there was a fix in rails.

Ruby on Rails member

I know of an application that reports lost connections often. It is a busy application (about 40K rpm). A priori seems strange because the connection pool does a mysql_ping on checkout, so you got a successful ping and just milliseconds later the connection is lost (or the server gone, also happens). The MySQL server on the other hand seems to be doing fine.

Not saying it is related to this patch, just a followup.

tenderlove and others added some commits Mar 9, 2012

Awesome. Am hoping this helps solve a related problem I am trying to investigate in my app, causing it to require more connections in the pool than I think it ought to, since rails 3.0 even.

However, you now have two similar but different methods with similar but different names: release(conn) and release_connection(conn). Can you maybe put some inline comments in the new release explaining how it's different than release_connection

Alternately, do they need to be two different methods? The new release seems to possibly do a superset of what the old release_connection did -- should they just be combined?

Ruby on Rails member

release_connection actually takes the thread id associated with the connection. I think it's meant to be called with no arguments. release is meant to be called with a connection, and it's private. We could probably combine release_connection and release, but I wanted to make the smallest change possible. Fix bug in one commit, refactor later. :-)

parndt and others added some commits Mar 10, 2012
Ruby on Rails member

Is there a ticket associated with this commit? Seems it breaks existing behavior as noted in #5337

Ruby on Rails member

Tickets #790 and #5058.

Ruby on Rails member

Also this is why we have to do this: rack/rack#265

Ruby on Rails member

@pixeltrix makes sense. Thanks! ❤️

Ruby on Rails member


Ruby on Rails member

A test was pushed later iirc.

Ruby on Rails member



+1 to this. This is also problematic on old tables that were designed for HABTM relationships.


@tenderlove this is prob a dumb question, but just wanted to make sure that shift is intended for binds and not *binds? I noticed just now that this to_sql method will alter the binds param for anything that uses binds after this is called.

Ruby on Rails member

yes, the shift is indented for binds. Binds is a list of tuples, so the * is for expanding the tuple for the quote method.

Ruby on Rails member

@jrochkind this pull request has been cluttered with irrelevant commits and comments, if you still think this should be merged into rails, can you please create a new pool request? I will close this one as it does not make any sense to go through that.

@drogus drogus closed this May 11, 2012

how the heck did all those comments and commits that have nothing to do with this issue get on here? Github Issue fail.

In fact, for the record, for anyone trying to figure out what happened with the actual issue at the top of this page:

This issue was resolved in 3-2-stable with... a pull request I can not find in Github Issues anymore. Something weird as heck is going on in Github Issues.

Okay, here's the specific commit that resolved this issue in 3-2-stable. 41563b4

master/rails4 had diverged at that point, and @tenderlove was rethinking how to handle things, so that patch (or similar) was not made to master.

At present as I write this, I believe the problem does still exist in master/rails4 (possibly even worse).


this is triggering a deprecation warning for associations when using the assignment operator (record.association_name = [records])


+3 should we ignore these warning on associations or handle them somehow differently?


You are crazy guys! You haven't even mentioned such an important change on the changelog!
I have spent three days to solve a problem with Redmine upgrade!
Have a look:

Ruby on Rails member

@giner Please feel free to send a pull request with a changelog entry if that's missing, we're trying to ensure nothing goes without a changelog now. Thanks.

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