-
Notifications
You must be signed in to change notification settings - Fork 1
OCTO-367 allow active record 7.1 #18
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
base: v0.x-master
Are you sure you want to change the base?
Changes from 11 commits
7b54cd4
f0f958e
2a36579
0a99266
d82551c
f169f30
b2c5089
ea193b9
ad9fe3b
fa7924a
89ed20d
6c6f85c
2e08fa6
4ff7d7c
3bc863d
7b3671d
cb90a78
886ce61
6c51dda
76ddbe7
645cbfa
13afedc
05fd774
5acdde2
879f492
696d2a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,12 +3,12 @@ | |
| specs: | ||
| fibered_mysql2 (0.3.1) | ||
| em-synchrony (~> 1.0) | ||
| rails (>= 6.1, < 7.1) | ||
| rails (>= 7.0, < 7.2) | ||
|
|
||
| GEM | ||
| remote: https://rubygems.org/ | ||
| specs: | ||
| actioncable (7.0.8.6) | ||
|
Check failure on line 11 in Gemfile.lock
|
||
| actionpack (= 7.0.8.6) | ||
| activesupport (= 7.0.8.6) | ||
| nio4r (~> 2.0) | ||
|
|
@@ -19,7 +19,7 @@ | |
| activerecord (= 7.0.8.6) | ||
| activestorage (= 7.0.8.6) | ||
| activesupport (= 7.0.8.6) | ||
| mail (>= 2.7.1) | ||
|
Check failure on line 22 in Gemfile.lock
|
||
| net-imap | ||
| net-pop | ||
| net-smtp | ||
|
|
@@ -35,17 +35,17 @@ | |
| rails-dom-testing (~> 2.0) | ||
| actionpack (7.0.8.6) | ||
| actionview (= 7.0.8.6) | ||
| activesupport (= 7.0.8.6) | ||
|
Check failure on line 38 in Gemfile.lock
|
||
| rack (~> 2.0, >= 2.2.4) | ||
| rack-test (>= 0.6.3) | ||
| rails-dom-testing (~> 2.0) | ||
|
Check failure on line 41 in Gemfile.lock
|
||
| rails-html-sanitizer (~> 1.0, >= 1.2.0) | ||
| actiontext (7.0.8.6) | ||
| actionpack (= 7.0.8.6) | ||
| activerecord (= 7.0.8.6) | ||
| activestorage (= 7.0.8.6) | ||
| activesupport (= 7.0.8.6) | ||
| globalid (>= 0.6.0) | ||
|
Check failure on line 48 in Gemfile.lock
|
||
| nokogiri (>= 1.8.5) | ||
| actionview (7.0.8.6) | ||
| activesupport (= 7.0.8.6) | ||
|
|
@@ -75,7 +75,7 @@ | |
| tzinfo (~> 2.0) | ||
| appraisal (2.5.0) | ||
| bundler | ||
| rake | ||
|
Check failure on line 78 in Gemfile.lock
|
||
| thor (>= 0.14.0) | ||
| appraisal-matrix (0.3.0) | ||
| appraisal (~> 2.2) | ||
|
|
@@ -92,7 +92,7 @@ | |
| tins (~> 1.6) | ||
| crass (1.0.6) | ||
| date (3.3.4) | ||
| diff-lcs (1.5.1) | ||
| diff-lcs (1.6.2) | ||
| docile (1.4.1) | ||
| em-synchrony (1.0.6) | ||
| eventmachine (>= 1.0.0.beta.1) | ||
|
|
@@ -169,19 +169,19 @@ | |
| thor (~> 1.0) | ||
| zeitwerk (~> 2.5) | ||
| rake (13.2.1) | ||
| rspec (3.13.0) | ||
| rspec (3.13.1) | ||
| rspec-core (~> 3.13.0) | ||
| rspec-expectations (~> 3.13.0) | ||
| rspec-mocks (~> 3.13.0) | ||
| rspec-core (3.13.0) | ||
| rspec-core (3.13.5) | ||
| rspec-support (~> 3.13.0) | ||
| rspec-expectations (3.13.1) | ||
| rspec-expectations (3.13.5) | ||
| diff-lcs (>= 1.2.0, < 2.0) | ||
| rspec-support (~> 3.13.0) | ||
| rspec-mocks (3.13.1) | ||
| rspec-mocks (3.13.5) | ||
| diff-lcs (>= 1.2.0, < 2.0) | ||
| rspec-support (~> 3.13.0) | ||
| rspec-support (3.13.1) | ||
| rspec-support (3.13.4) | ||
| simplecov (0.16.1) | ||
| docile (~> 1.1) | ||
| json (>= 1.8, < 3) | ||
|
|
@@ -218,4 +218,4 @@ | |
| rspec | ||
|
|
||
| BUNDLED WITH | ||
| 2.2.29 | ||
| 2.6.9 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,5 +12,6 @@ gem "pry-byebug" | |
| gem "rake" | ||
| gem "rspec" | ||
| gem "rails", "~> 7.0.0" | ||
| gem "mutex_m" | ||
|
|
||
| gemspec path: "../" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,12 @@ | |
| require 'em-synchrony' | ||
| require 'active_model' | ||
| require 'active_record/errors' | ||
|
|
||
| require 'active_record/connection_adapters/mysql2_adapter' | ||
| require 'active_record/connection_adapters/em_mysql2_adapter' | ||
| require 'em-synchrony/mysql2' | ||
|
|
||
| module FiberedMysql2 | ||
| module FiberedMysql2Adapter_5_2 | ||
| module FiberedMysql2Adapter_7_0 | ||
| def lease | ||
| if in_use? | ||
| msg = "Cannot lease connection, ".dup | ||
|
|
@@ -62,8 +63,10 @@ def owner_fiber | |
| end | ||
| end | ||
|
|
||
| class FiberedMysql2Adapter < ::ActiveRecord::ConnectionAdapters::EMMysql2Adapter | ||
| include FiberedMysql2Adapter_5_2 | ||
| class FiberedMysql2Adapter < ::ActiveRecord::ConnectionAdapters::Mysql2Adapter | ||
| if ::ActiveRecord.gem_version < "7.1" | ||
| include FiberedMysql2Adapter_7_0 | ||
| end | ||
|
|
||
| class << self | ||
| # Copied from Mysql2Adapter, except with the EM Mysql2 client | ||
|
|
@@ -77,9 +80,5 @@ def new_client(config) | |
| end | ||
| end | ||
| end | ||
|
|
||
| def initialize(*args) | ||
| super | ||
| end | ||
|
Comment on lines
-81
to
-83
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant |
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -184,33 +184,36 @@ def mon_exit_for_cond | |
| end | ||
|
|
||
| module FiberedDatabaseConnectionPool | ||
| include FiberedMonitorMixin | ||
|
|
||
| module Adapter_5_2 | ||
| def cached_connections | ||
| @thread_cached_conns | ||
| module Adapter_7_0 | ||
| def release_connection(owner_thread = Fiber.current) | ||
| if (conn = @thread_cached_conns.delete(connection_cache_key(owner_thread))) | ||
| checkin(conn) | ||
| end | ||
| end | ||
|
|
||
| def current_connection_id | ||
| connection_cache_key(current_thread) | ||
| def with_connection | ||
| unless (conn = cached_connections[current_connection_id]) # Invoca Patch to use Fiber | ||
| conn = connection | ||
| fresh_connection = true | ||
| end | ||
| yield conn | ||
| ensure | ||
| release_connection if fresh_connection | ||
| end | ||
|
|
||
| def checkout(checkout_timeout = @checkout_timeout) | ||
| begin | ||
| reap_connections | ||
| rescue => ex | ||
| ActiveRecord::Base.logger.error("Exception occurred while executing reap_connections: #{ex}") | ||
| end | ||
| super | ||
| def current_thread | ||
| Fiber.current | ||
| end | ||
|
|
||
| def release_connection(owner_thread = Fiber.current) | ||
| if (conn = @thread_cached_conns.delete(connection_cache_key(owner_thread))) | ||
| checkin(conn) | ||
| end | ||
| def connection | ||
| cached_connections[current_connection_id] ||= checkout | ||
| end | ||
| end | ||
| include Adapter_5_2 | ||
|
|
||
| if ::ActiveRecord.gem_version < "7.1" | ||
| include Adapter_7_0 | ||
| end | ||
| include FiberedMonitorMixin # This is switches the connection pool's mutex and condition variables to event machine / Fiber compatible ones. | ||
|
|
||
| def initialize(pool_config) | ||
| if pool_config.db_config.reaping_frequency | ||
|
|
@@ -222,41 +225,23 @@ def initialize(pool_config) | |
| @reaper = nil # no need to keep a reference to this since it does nothing in this sub-class | ||
| end | ||
|
|
||
| def connection | ||
| # this is correctly done double-checked locking | ||
| # (ThreadSafe::Cache's lookups have volatile semantics) | ||
| if (result = cached_connections[current_connection_id]) | ||
| result | ||
| else | ||
| synchronize do | ||
| if (result = cached_connections[current_connection_id]) | ||
| result | ||
| else | ||
| cached_connections[current_connection_id] = checkout | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This double check isn't necessary since the This is a legacy of EM::Synchrony's code that was making the connection pool safe to use one fiber across the single thread. |
||
|
|
||
| def reap_connections | ||
| cached_connections.values.each do |connection| | ||
| unless connection.owner.alive? | ||
| checkin(connection) | ||
| end | ||
| end | ||
|
Comment on lines
-241
to
-246
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method is our own method only used in |
||
| def current_connection_id | ||
| connection_cache_key(current_thread) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| #-- | ||
| # This hook-in method allows for easier monkey-patching fixes needed by | ||
| # JRuby users that use Fibers. | ||
| def connection_cache_key(fiber) | ||
| fiber | ||
| def cached_connections | ||
| @thread_cached_conns | ||
| end | ||
|
|
||
| def current_thread | ||
| Fiber.current | ||
| # Invoca patch that reaps orphaned connections on checkout. This lets us immediately use a connection left open by dead fibers | ||
| # instead of waiting for all connections to be used in the pool before they are reaped. | ||
| def checkout(checkout_timeout = @checkout_timeout) | ||
| begin | ||
| reap | ||
| rescue => ex | ||
| ActiveRecord::Base.logger.error("Exception occurred while executing reap_connections: #{ex}") | ||
| end | ||
| super | ||
| end | ||
| end | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,101 +2,6 @@ | |
|
|
||
| require_relative '../active_record/connection_adapters/fibered_mysql2_adapter' | ||
|
|
||
| module EM::Synchrony | ||
| module ActiveRecord | ||
| _ = Adapter_4_2 | ||
| module Adapter_4_2 | ||
|
Comment on lines
-5
to
-8
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed the inclusion of this module as its not needed. The TransactionManager is configured per connection, and the isolating the TransactionManager per Fiber is only necessary if you're sharing the connection across multiple fibers which we're not. |
||
| def configure_connection | ||
| super # undo EM::Synchrony's override here | ||
| end | ||
|
|
||
| def transaction(*args) | ||
| super # and here | ||
| end | ||
|
|
||
| _ = TransactionManager | ||
| class TransactionManager < _ | ||
| # Overriding the em-synchrony override to bring it up to rails 6 requirements. | ||
| # Changes from the original Rails 6 source are: | ||
| # 1. the usage of _current_stack created by em-synchrony instead of the Rails provided @stack instance variable | ||
| # 2. the usage of Fiber.current.object_id as a part of the savepoint transaction name | ||
| # | ||
| # Original EM Synchrony Source: | ||
| # https://github.com/igrigorik/em-synchrony/blob/master/lib/em-synchrony/activerecord_4_2.rb#L35-L44 | ||
| # | ||
| # Original Rails Source: | ||
| # https://github.com/rails/rails/blob/6-0-stable/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb#L205-L224 | ||
| def begin_transaction(isolation: nil, joinable: true, _lazy: true) | ||
| @connection.lock.synchronize do | ||
| run_commit_callbacks = !current_transaction.joinable? | ||
| transaction = | ||
| if _current_stack.empty? | ||
| ::ActiveRecord::ConnectionAdapters::RealTransaction.new(@connection, isolation:, joinable:, run_commit_callbacks: run_commit_callbacks) | ||
| else | ||
| ::ActiveRecord::ConnectionAdapters::SavepointTransaction.new(@connection, "active_record_#{Fiber.current.object_id}_#{open_transactions}", _current_stack.last, isolation:, joinable:, run_commit_callbacks: run_commit_callbacks) | ||
| end | ||
|
|
||
| if @connection.supports_lazy_transactions? && lazy_transactions_enabled? && _lazy | ||
| @has_unmaterialized_transactions = true | ||
| else | ||
| transaction.materialize! | ||
| end | ||
| _current_stack.push(transaction) | ||
| transaction | ||
| end | ||
| end | ||
|
|
||
| # Overriding the ActiveRecord::TransactionManager#materialize_transactions method to use | ||
| # fiber safe the _current_stack instead of the @stack instance variable. when marterializing | ||
| # transactions. | ||
| def materialize_transactions | ||
| return if @materializing_transactions | ||
| return unless @has_unmaterialized_transactions | ||
|
|
||
| @connection.lock.synchronize do | ||
| begin | ||
| @materializing_transactions = true | ||
| _current_stack.each { |t| t.materialize! unless t.materialized? } | ||
| ensure | ||
| @materializing_transactions = false | ||
| end | ||
| @has_unmaterialized_transactions = false | ||
| end | ||
| end | ||
|
|
||
| # Overriding the ActiveRecord::TransactionManager#commit_transaction method to use | ||
| # fiber safe the _current_stack instead of the @stack instance variable. when marterializing | ||
| # transactions. | ||
| def commit_transaction | ||
| @connection.lock.synchronize do | ||
| transaction = _current_stack.last | ||
|
|
||
| begin | ||
| transaction.before_commit_records | ||
| ensure | ||
| _current_stack.pop | ||
| end | ||
|
|
||
| transaction.commit | ||
| transaction.commit_records | ||
| end | ||
| end | ||
|
|
||
| # Overriding the ActiveRecord::TransactionManager#rollback_transaction method to use | ||
| # fiber safe the _current_stack instead of the @stack instance variable. when marterializing | ||
| # transactions. | ||
| def rollback_transaction(transaction = nil) | ||
| @connection.lock.synchronize do | ||
| transaction ||= _current_stack.pop | ||
| transaction.rollback | ||
| transaction.rollback_records | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| module FiberedMysql2 | ||
| module FiberedMysql2ConnectionFactory | ||
| def fibered_mysql2_connection(raw_config) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't actually providing us anything useful:
https://github.com/igrigorik/em-synchrony/blob/master/lib/active_record/connection_adapters/em_mysql2_adapter.rb#L40-L46
We aren't actually using the EMMysql2Adapter::Client, and the code in
EM::Synchrony::ActiveRecord::Adapter_4_2actually isn't necessary either.https://github.com/igrigorik/em-synchrony/blob/master/lib/em-synchrony/activerecord_4_2.rb
The only thing this was doing was converting the
TransactionManagerto the Fiber isolated TransactionManager, however this isn't necessary as the TransactionManager lives on the Connection, and for FiberedMysql2, we already are isolating the connections per Fiber.Also if we call
This has a requires a lock on the connection to enter the transaction so this is already safe across fibers as another fiber even if it had access to the connection for some reason would not be able to update the state of the TransactionManager