Skip to content
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

Really fiber-aware ActiveRecord connection pool and em_mysql2 adapter #179

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dmgtn
Copy link
Contributor

@dmgtn dmgtn commented Feb 9, 2014

This implementation supports concurrent running transactions. We tested it with active_record 3.2, but did not test with 4.0. We are using it in production and it is working well.

ActiveRecord have already had good connection pooling, that uses Thread.current[] for thread-aware (and fiber-aware) storage. We replaced original MonitorMixin, by fiber-aware one, and thats all.

@igrigorik
Copy link
Owner

Looks to be much simpler implementation, which is a good sign. That said, would be great to get 4.0 support! Any chance you could give it a shot and see what the delta is (if any)?

@marshall-lee
Copy link

@mrms-dos @igrigorik
Are you still here ??

I have some questions

Am I right that everything should work without defining custom adapters if we just:

$VERBOSE.tap do |old_verbose|
  $VERBOSE = nil
  Mysql2::Client = Mysql2::EM::Client
  $VERBOSE = old_verbose
end

It's really dirty but doing copy-paste of some of the Mysql2Adapter implementation is dirtier :)
And I don't think that someone would want both mysql2 and em_mysql2 in the same project.

@marshall-lee
Copy link

What was the motivation of stubbing clear_stale_cached_connections! ?

UPD: In ActiveRecord 4.x clear_stale_cached_connections! is replaced with reap.

  • In 4.0 and 4.1 reap is not been calling by default from checkout like clear_stale_cached_connections!. It is been calling by separate Reaper thread that calls reap periodically but only if reaping_frequency is set in database.yml.
  • In 4.2 it's been calling from both acquire_connection and Reaper thread (which is optional, again).

I think we should not stub reap in ActiveRecord 4.x because it's thread-agnostic now unlike clear_stale_cached_connections!.

We also can implement Reaper differently: use EM timers instead of thread with sleep.

module ActiveRecord
  module ConnectionAdapters
    class ConnectionPool
      if ActiveRecord::VERSION::MAJOR == 4
        class Reaper
          def run
            return unless frequency
            EM::Synchrony.add_periodic_timer(frequency) do
              pool.reap
            end
          end
        end
      end
    end
  end
end

It would be useful! But we cannot backport it to ActiveRecord 3.x, so clear_stale_cached_connections! must be just stubbed like in this PR.

@marshall-lee
Copy link

ActiveRecord::Base.connection_id ||= Fiber.current.object_id

Taking Fiber.current.object_id is right but Base.connection_id is still saved per thread. Is it potentially harmful?

UPD: sorry, i just realized that Thread.current is also fiber-local.

@marshall-lee
Copy link

Now i'm trying to use em-pg-client with ActiveRecord 4.2 and ConnectionPool with fiber aware MonitorMixin from this PR. Things are almost the same at first sight and it should work.

The little difference coming from 4.2 is:

module ActiveRecord
  module ConnectionAdapters
    class AbstractAdapter
      include EventMachine::Synchrony::MonitorMixin

      if [ActiveRecord::VERSION::MAJOR, ActiveRecord::VERSION::MINOR] == [4, 2]
        def lease
          synchronize do
            unless in_use?
              @owner = Fiber.current
            end
          end
        end
      end
    end
  end
end

For PostgreSQL users:

require 'em-pg-client'

$VERBOSE.tap do |old_verbose|
  $VERBOSE = nil
  PGconn = PG::EM::Client
  $VERBOSE = old_verbose
end

@marshall-lee
Copy link

Please see #190

@dgutov
Copy link
Collaborator

dgutov commented Mar 16, 2015

@mrms-dos @marshall-lee Please merge the current master in your pull request branches, or rebase on top of it, so that they build on Travis, and we can see how the specs pass on different ActiveRecord versions.

@marshall-lee
Copy link

@dgutov did it in #190

@igrigorik
Copy link
Owner

Looks like this and #190 are duplicates. Any objections to closing this one in favor of #190 -- seems like we're farther ahead there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants