OCTO-367 allow active record 7.1 #18
Conversation
| end | ||
| end | ||
|
|
||
| class FiberedMysql2Adapter < ::ActiveRecord::ConnectionAdapters::EMMysql2Adapter |
There was a problem hiding this comment.
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_2 actually 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 TransactionManager to 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
ActiveRecord::Base.transaction do
# query stuff ...
endThis 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
| def initialize(*args) | ||
| super | ||
| 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 |
There was a problem hiding this comment.
This double check isn't necessary since the current_connection_id is the current Fiber and we'd only ever be setting this value in the cached_connections in this fiber as well.
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 |
There was a problem hiding this comment.
This method is our own method only used in checkout override, which I've replaced with just calling reap.
| module EM::Synchrony | ||
| module ActiveRecord | ||
| _ = Adapter_4_2 | ||
| module Adapter_4_2 |
There was a problem hiding this comment.
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.
| expect(c0.in_use?).to be | ||
| expect(c1.in_use?).to be |
There was a problem hiding this comment.
.in_use? is an alias for owner so we're already checking this above.
| expect(client).to receive(:query) { }.exactly(2).times | ||
|
|
||
| reap_connection_count = Rails::VERSION::MAJOR > 4 ? 5 : 3 | ||
| expect(ActiveRecord::Base.connection_pool).to receive(:reap_connections).with(no_args).exactly(reap_connection_count).times.and_call_original |
There was a problem hiding this comment.
This expect on the query isn't a meaningful test and changes across rails versions. The main test is below.
| end | ||
|
|
||
| context '#lease' do | ||
| context '#lease', if: ActiveRecord.gem_version < "7.1" do |
There was a problem hiding this comment.
These overrides are only necessary in Rails 7.0
ttstarck
requested review from
a team and
yinonrousso
and removed request for
a team
ttstarck
force-pushed
the
OCTO-367_allow_active_record_7_1
branch
from
8a642ee to
645cbfa
Compare
1 participant
Uh oh!
There was an error while loading. Please reload this page.