OCTO-367 remove usage of em-synchrony active record adapter #19
Conversation
ttstarck
force-pushed
the
OCTO-367_remove_usage_of_em_synchrony_adapter
branch
from
20cb902 to
10faec8
Compare
| @@ -1,6 +1,6 @@ | |||
| --- | |||
| name: FiberedMySQL2 Gem Build | |||
| on: [push, pull_request] | |||
There was a problem hiding this comment.
pull_request was seemingly causing two builds to run at the same time. Removing it fixed the issue.
| gem 'coveralls', require: false | ||
| gem 'mysql2', '~> 0.5' | ||
| gem 'nokogiri' | ||
| gem 'pry' | ||
| gem 'pry-byebug' | ||
| gem 'rake' | ||
| gem 'rspec' |
There was a problem hiding this comment.
I am consistently getting random segfaults issues with coveralls and rspec mocks. Rspec 3.12 seems to be a little better for the segfaults but I think this is an issue with EventMachine as well...
Also coveralls is no longer supported so I removed it.
| end | ||
| end | ||
|
|
||
| class FiberedMysql2Adapter < ::ActiveRecord::ConnectionAdapters::Mysql2Adapter |
There was a problem hiding this comment.
Notice here that we are no longer inheriting from ::ActiveRecord::ConnectionAdapters::EMMysql2Adapter.
This adapter came from em-synchrony gem, however the adapter was built for Rails 4 and has never been updated. We've had multiple methods that were getting patched by the em-synchrony adapter that we had to unpatch (see #initialize method below as one example). The em-synchroney adapter was not providing much use for us anymore so I removed it, which also meant we could remove several other patches.
| # Because we are actively releasing connections from dead fibers, we only want | ||
| # to enforce that we're expiring the current fibers connection, iff the owner | ||
| # of the connection is still alive. | ||
| if @owner.alive? && @owner != ActiveSupport::IsolatedExecutionState.context |
There was a problem hiding this comment.
Invoca patch is to also check if the owner is alive first. This is only necessary because of our reap_connections custom method. Once we replace the custom reap_connections method with the standard #reap method, we can remove this patch.
| def initialize(*args) | ||
| super | ||
| end |
There was a problem hiding this comment.
This was only necessary when we were inheriting from ActiveRecord::ConnectionAdapters::EMMysql2Adapter
| module EM::Synchrony | ||
| module ActiveRecord | ||
| _ = Adapter_4_2 | ||
| module Adapter_4_2 |
There was a problem hiding this comment.
All of this code is now not necessary because our FiberedMysql2Adapter class is no longer inheriting from the ActiveRecord::ConnectionAdapters::EMMysql2Adapter
| end | ||
|
|
||
| _ = TransactionManager | ||
| class TransactionManager < _ |
There was a problem hiding this comment.
This TransactionManager patch from EM::Synchrony made it so that the transaction stack is isolated per fiber.
Our patches just brought the EM::Synchrony's patches up to the modern rails version.
However, the TransactionManager instance belongs to the Connection Adapter instance, and we are already isolated per Fiber at the connection (adapter) level. That means all of this code to patch the transaction manager is not needed. Hence why in Rails 7.1 and beyond, they never made the TransactionManager's stack fiber isolated. Also, TransactionManager instance uses connection.lock.synchronize whenever it changes state so this is Thread and Fiber safe.
| expect(client).to_not receive(:query).with("BEGIN") | ||
| expect(client).to_not receive(:query).with("COMMIT") |
There was a problem hiding this comment.
This was causing segfaults... I'm blaming EventMachine... I don't know...
| 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 isn't needed to properly assert the connections are reclaimed after the fiber exits.
| it "should serve separate connections per fiber" do | ||
| expected_query = if Rails::VERSION::MAJOR > 4 | ||
| "SET @@SESSION.sql_mode = CONCAT(CONCAT(@@sql_mode, ',STRICT_ALL_TABLES'), ',NO_AUTO_VALUE_ON_ZERO'), @@SESSION.sql_auto_is_null = 0, @@SESSION.wait_timeout = 2147483" | ||
| else | ||
| "SET @@SESSION.sql_auto_is_null = 0, @@SESSION.wait_timeout = 2147483, @@SESSION.sql_mode = 'STRICT_ALL_TABLES'" | ||
| end | ||
| expect(client).to receive(:query) do |*args| | ||
| expect(args).to eq([expected_query]) | ||
| end.exactly(2).times |
There was a problem hiding this comment.
This is not necessary to assert that we serve different connections per fiber.
ttstarck
requested review from
a team,
jebentier and
yinonrousso
and removed request for
a team
Support Rails 7.1!