Non-threadsafe database connections shared between threads

[bwbuchanan]

When running @javascript scenarios, the database connection adapter is shared between cucumber and the webserver. This is completely unsafe, and leads to some very confusing and difficult-to-diagnose bugs when both threads attempt to run queries simultaneously. With PostgreSQL, I have seen one thread get the response to the other thread's database query, which generally hoses everything up.

This bug was introduced by commit cbcfd88.

[jdelStrother]

Hi - I spent a really long time tracking down a bug caused by this, and would agree that the current behaviour needs changing.

It's only necessary so that DatabaseCleaner.strategy = :transaction works with selenium, right? I think that transactions & selenium's threaded approach are mutually incompatible - would we be better off warning people to just use :truncation in @javascript scenarios instead?

[jasonm]

I was referred here from rails/rails#2943 (comment) - very excited to see this, it seems like a lead on our issue.

If it is the same issue, we were seeing it with :truncation as well.

There's a sample app attached to that ticket which (intermittently, but frequently) reproduces this issue.

[jasonm]

I was originally confused since Capybara forks the Rails server https://github.com/jnicklas/capybara/blob/master/lib/capybara/server.rb#L64-66 which, when you inspect ObjectSpace, results in two PGconn objects. But, because of cbcfd88 only one PGconn instance is actually used.

[jdelStrother]

I've fixed it in my app with the following in env.rb :

  DatabaseCleaner.strategy = :truncation
  Before('@javascript') do
    ActiveRecord::Base.shared_connection = nil
  end

which disables the cross-thread connection entirely. Using :truncation rather than :transaction is necessary because you no longer have 1 connection across 2 threads, so changes in one thread wouldn't be seen in the other.

[bwbuchanan]

It would be terribly hacky, but I think this could be made to work the way it was intended by wrapping the database connection with a proxy object that serializes access to it. The proxy would have to be smart enough to keep track of transactions (including nested transactions) and only permit another process to take the connection if no transaction was running.

Actually, on second thought, it would still be unsafe, because one process could change some per-connection state (using SET <configuration_parameter> = ), and the other process might not be expecting it.

[ClayShentrup]

Any updates on this? We can't upgrade to Rails 3.1 because of it. Grrr.

[aslakhellesoy]

@BrokenLadder quit whining and submit a bugfix.

[ClayShentrup]

Translation:

I have no information indicating that a fix from myself or the other major contributors is imminent.

If you would like to look into this and fix the bug, the community would appreciate it.

[jdelStrother]

Hi - maybe I'm seeing a different bug, but as far as I can tell this bug has nothing to do with Rails 3.1 - I see its symptoms in 3.0.

@aslakhellesoy - I'll happily submit a bugfix, but it would more-or-less consist of "git revert cbcfd88" : I don't think the cross-thread connection is a workable solution. Want me to do so?

[ClayShentrup]

Rails 3.1 depends on a higher version of cucumber-rails, which seems to have this bug. You could of course also get this bug in Rails 3.0 by installing that version of cucumber-rails.

[ClayShentrup]

I had what I thought was the same issue, and this was the offending commit. Reverting it got my tests green again.

30e5e36

UPDATE: But then I upgraded from 3.0 to 3.1 and now I have this problem again.

[DanThiffault]

Had this same problem. Rewriting tests so that factories were all called before selenium seems to be a viable workaround for the moment.

[mike-burns]

The above fix was close but not quite. Here's closer:

begin
  DatabaseCleaner.strategy = :truncation
rescue NameError
  raise "You need to add database_cleaner to your Gemfile (in the :test group) if you wish to use it."
end
Before('@javascript') do
  ActiveRecord::Base.shared_connection = nil
  ActiveRecord::Base.descendants.each do |model|
    model.shared_connection = nil
  end
end

This now patches around the commit, which should obviously be reverted.

[ClayShentrup]

This descendants line does nothing, because shared_connection is a class_attribute. You're just setting the same thing over and over again for every descendant.

[sorentwo]

It definitely does something, including that line just got my suite working with postgres again.

[ClayShentrup]

Well I'll be. It seems I misunderstood what class_attribute does.

as long as Subclass does not assign a value to setting by performing Subclass.setting = something , Subclass.setting would read value assigned to parent class. Once Subclass assigns a value then the value assigned by Subclass would be returned.

I had read this post by Obie Fernandez and just assumed that because setting it for class also set it for subclass, that setting it for subclass would also set it for class. Wrong. Here's the post though, for reference.

[sorentwo]

After digesting the connection management post by @tenderlove it makes sense why this would be necessary.

Lets hope this gets reverted and connection management gets cleaned up shortly.

Edit: Update "connection management post" link

[ClayShentrup]

@sorentwo

Sounds plausable ;)

[pisaruk]

The solution proposed by mike-burns worked for me. Tks.

[mattwynne]

Phew. What a confusing mess! So, let me see if I've got this straight:

1. We can't use transactional database rollback unless everything happens through the same database connection
2. Using the same database connection is safe enough for rack-based scenarios because everything is synchronous and runs in a single thread (assuming you don't use threads in your actual application).
3. Using the same database connection is not safe for @javascript scenarios because Cucumber and the web server run in separate threads and thus could stomp on each other as they access the database connection.

I'm curious how this works with other testing tools? What do you do if you're using Capybara with RSpec integration tests to drive selenium, for example? How does Rails' bulit-in transactional_fixtures stuff work these days?

[mattwynne]

A little bit of background:
https://groups.google.com/forum/#!msg/ruby-capybara/JI6JrirL9gM/R6YiXj4gi_UJ

At the time @jnicklas wrote that post, cucumber-rails used to switch database strategies and go for truncation whenever there was a @javascript scenario. Are we saying we need to go back to that?

[jdelStrother]

@mattwynne - yep, your summary is correct. In my opinion, we ought to avoid transactional database strategies with javascript and revert the shared connection behaviour.

[mattwynne]

Can someone please try out this patch? If it works, you should be able to configure cucumber-rails to use truncation for js scenarios using:

Cucumber::Rails::Database.javascript_strategy = :truncation

Just pop that somewhere in your features/support folder after you've required cucumber-rails.

Using a strategy allows people who are prepared to take the risk of shared connections for the payoff of faster tests, the rest of you can have something thread-safe. I would have made truncation the default, but it seems the current tests are dependent on the shared connections stuff and I don't have the energy to change them right now. Patches to the tests are most welcome!

[mattwynne]

There's a feature for it now which explains how to use it (and caused me to actually make it work!).

I still think we should make truncation the default: the shared connection hack is useful if you know what you're doing but we don't want to give anyone else a sleepless night.

Feedback?

[jnicklas]

I think that wrapping the entire request in a Mutex should do the trick. Rails does this anyway by default, unless you switch on threadsafe! mode. I've personally used this on tons of projects and have never had any kinds of issues with it. To the people who have had problems, are you running Rails in threadsafe mode?

That being said, I agree that shared connections should probably not be the default. It's a fine enhancement if you know the pitfalls, but we should not be making that choice for others.

[jdelStrother]

There are situations where you don't control the request though - for example, if you open a page that then repeatedly polls the server for a database change. If you then combine that with cucumber touching the database while that polling page is open, you're fairly guaranteed to run into threading problems.

On 3 Nov 2011, at 12:01, Jonas [email protected] wrote:

I think that wrapping the entire request in a Mutex should do the trick. Rails does this anyway by default, unless you switch on threadsafe! mode. I've personally used this on tons of projects and have never had any kinds of issues with it. To the people who have had problems, are you running Rails in threadsafe mode?

That being said, I agree that shared connections should probably not be the default. It's a fine enhancement if you know the pitfalls, but we should not be making that choice for others.

Reply to this email directly or view it on GitHub:
#166 (comment)

[jnicklas]

@jdelStrother hmm, I guess you would have to put all interaction with the DB in your specs in a Mutex as well then, not ideal obviously.

[aslakhellesoy]

Merged to master. Awesome fix Matt!!!

[ClayShentrup]

Does this solution address @jdelStrother's point?

[aslakhellesoy]

@BrokenLadder - I think @jdelStrother's point was that it's not always safe to use the same connection instance from two different threads.

The default for @javascript scenarios is now to have a different connection instance per thread (or process). Cucumber in the main thread has one connection - The app in the other thread has its own instance.

In other words - his point is addressed.

[jdelStrother]

Yep, Matt's stuff fixes my concerns. It would be good to see #182 fixed up, but everything else looks great.

[jasonm]

FYI for older versions upgrading to versions which include this patch, since Cucumber::Rails::Database.javascript_strategy = :truncation is emitted in a generator, users will have to add that line by hand, else they get:

 undefined method `before_js' for nil:NilClass (NoMethodError)
 /Users/jason/.rvm/gems/ruby-1.9.2-p290/gems/activesupport-3.1.0/lib/active_support/whiny_nil.rb:48:in `method_missing'
 /Users/jason/.rvm/gems/ruby-1.9.2-p290/bundler/gems/cucumber-rails-192768e5dc29/lib/cucumber/rails/database.rb:12:in `before_js'
 /Users/jason/.rvm/gems/ruby-1.9.2-p290/bundler/gems/cucumber-rails-192768e5dc29/lib/cucumber/rails/hooks/active_record.rb:11:in `Before'

Alternatively we could provide a default value for @strategy inside Cucumber::Rails::Database in cucumber-rails.

[mattwynne]

On 4 Nov 2011, at 15:13, Jason Morrison wrote:

FYI for older versions upgrading to versions which include this patch, since Cucumber::Rails::Database.javascript_strategy = :truncation is emitted in a generator, users will have to add that line by hand, else they get:

undefined method `before_js' for nil:NilClass (NoMethodError)
/Users/jason/.rvm/gems/ruby-1.9.2-p290/gems/activesupport-3.1.0/lib/active_support/whiny_nil.rb:48:in `method_missing'
/Users/jason/.rvm/gems/ruby-1.9.2-p290/bundler/gems/cucumber-rails-192768e5dc29/lib/cucumber/rails/database.rb:12:in `before_js'
/Users/jason/.rvm/gems/ruby-1.9.2-p290/bundler/gems/cucumber-rails-192768e5dc29/lib/cucumber/rails/hooks/active_record.rb:11:in `Before'

Alternatively we could provide a default value for @strategy inside Cucumber::Rails::Database in cucumber-rails.

Yes, this needs fleshing out with specs on Cucumber::Rails::Database under #182. I'd love some help as I'm super busy for the next couple of weeks.

[heisee]

Oh man, over the last months it took us several hours of debugging in the transaction handling of database_statements.rb, since we sometimes got stacktraces like this:

ActiveRecord::JDBCError: SAVEPOINT active_record_1 does not exist: ROLLBACK TO SAVEPOINT active_record_1
arjdbc/jdbc/RubyJdbcConnection.java:191:in `execute'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-jdbc-adapter-1.2.2/lib/arjdbc/jdbc/adapter.rb:219:in `_execute'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-jdbc-adapter-1.2.2/lib/arjdbc/jdbc/adapter.rb:211:in `execute'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-3.2.1/lib/active_record/connection_adapters/abstract_adapter.rb:286:in `log'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activesupport-3.2.1/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activesupport-3.2.1/lib/active_support/notifications/instrumenter.rb:19:in `instrument'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-3.2.1/lib/active_record/connection_adapters/abstract_adapter.rb:281:in `log'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-jdbc-adapter-1.2.2/lib/arjdbc/jdbc/adapter.rb:211:in `execute'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-jdbc-adapter-1.2.2/lib/arjdbc/mysql/adapter.rb:156:in `rollback_to_savepoint'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-3.2.1/lib/active_record/connection_adapters/abstract/database_statements.rb:247:in `transaction'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-3.2.1/lib/active_record/connection_adapters/abstract/database_statements.rb:239:in `transaction'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-3.2.1/lib/active_record/connection_adapters/abstract/database_statements.rb:169:in `transaction'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-3.2.1/lib/active_record/transactions.rb:208:in `transaction'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-3.2.1/lib/active_record/transactions.rb:293:in `with_transaction_returning_status'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-3.2.1/lib/active_record/transactions.rb:246:in `save!'
/home/hk/.rvm/gems/jruby-1.6.7@us/gems/activerecord-3.2.1/lib/active_record/validations.rb:41:in `create!'

Finally we came to the point where we found the SharedConnectionStrategy. We didn't expect this, because we always thought, that the problem is, that the database_statements.rb is not multithreading-safe. After some frustration we found out, that the same DB connection is used (by "select connection_id()" on mysql) in some circumstances, but it took us quite some more time until we found the cause in conjunction with the @javascript tag.

We had a multithreaded backend and a frontend driven with selenium and use DatabaseCleaner with the :truncation strategy.
The fix for us was to put

module Cucumber
  module Rails
    module Database
      class SharedConnectionStrategy
        def before_js
        end
        def before_non_js
        end
      end
    end
  end
end

into env.rb

Maybe this helps others who google for "ROLLBACK TO SAVEPOINT active_record_1"

[KurtPreston]

I could not get this working at all, until implemented the following changes in my features/support/env.rb

Before '@javascript' do
  DatabaseCleaner.strategy = :truncation
end

Before '~@javascript' do
  DatabaseCleaner.strategy = :transaction
  DatabaseCleaner.start
end

After '~@javascript' do
  DatabaseCleaner.clean
end

[MarkDBlackwell]

@sorentwo, apparently tenderlove's October 2011 post on ActiveRecord connection management has moved here. (If you like, you might help readers by adjusting your comment above.)

BTW, @tenderlove refers us to some code in connection_specification.rb. Its version at that time is here.