Skip to content

Commit

Permalink
Fix preventing_writes for granular swapping
Browse files Browse the repository at this point in the history
When we implemented granular connection swapping we treated
`prevent_writes` similarly to `role` and `shard`. However, it's a little
different. `prevent_writes` is a feature of a conneciton whereas `role`
and `shared` are used to lookup a connection. A writing connection that
wants to prevent writes doesn't get stored in the pool as such,
`prevent_writes` is added as a way to change behavior of a connection.

Because of this when `preventing_writes?` calls
`ActiveRecord::Base.current_preventing_writes` the lookup is on the
wrong class if we're using granular swapping. Instead we need to lookup
the `prevent_writes` from the class that called `connected_to` rather
than `ActiveRecord::Base`. To do that I've done the following:

1) Added access to the `connection_specification_name` as `owner_name`
on the pool so the connection can access it.
2) Call `safe_constantize` on the `owner_name` since the
`connected_to_stack` stores the class as a class and not a string.
3) Now we can call `safe_constantize` on the klass that we got out of
the pool to be able to look up the correct `prevent_writes` from the
stack.
4) NullPool's get a nil connection specification name because if they're
not connected they can't prevent writes. This required changing some
tests but we don't support multi-db for a case where you're establishing
a connection directly from the adapter.

I benchmarked this and the two versions were pretty close, although this
is a little bit slower.

I tried a few other ways of fixing this. I first tried adding
`prevent_writes` to the connection in `retrieve_connection` based on the
`current_preventing_writes` but then that behavior only worked correctly
for ActiveRecord connections. If we already had a connection and
`retrieve_connection` isn't called then the tests testing that behavior
would fail. This was the best way to not introduce confusing differences
between global and granular connections.

Fixes #40559
  • Loading branch information
eileencodes committed Nov 17, 2020
1 parent 86a9c89 commit b92dc5b
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 14 deletions.
Expand Up @@ -23,6 +23,10 @@ class NullPool # :nodoc:
include ConnectionAdapters::AbstractPool

attr_accessor :schema_cache

def owner_name
nil
end
end

# Connection pool base class for managing Active Record database
Expand Down Expand Up @@ -356,7 +360,7 @@ def run
include ConnectionAdapters::AbstractPool

attr_accessor :automatic_reconnect, :checkout_timeout
attr_reader :db_config, :size, :reaper, :pool_config
attr_reader :db_config, :size, :reaper, :pool_config, :owner_name

delegate :schema_cache, :schema_cache=, to: :pool_config

Expand All @@ -371,6 +375,7 @@ def initialize(pool_config)

@pool_config = pool_config
@db_config = pool_config.db_config
@owner_name = pool_config.connection_specification_name

@checkout_timeout = db_config.checkout_timeout
@idle_timeout = db_config.idle_timeout
Expand Down
Expand Up @@ -113,14 +113,20 @@ def use_metadata_table?

# Determines whether writes are currently being prevents.
#
# Returns true if the connection is a replica, or if +prevent_writes+
# is set to true.
# Returns true if the connection is a replica.
#
# If the application is using legacy handling, returns
# true if `connection_handler.prevent_writes` is set.
#
# If the application is using the new connection handling
# will return true based on `current_preventing_writes`.
def preventing_writes?
if ActiveRecord::Base.legacy_connection_handling
replica? || ActiveRecord::Base.connection_handler.prevent_writes
else
replica? || ActiveRecord::Base.current_preventing_writes
end
return true if replica?
return ActiveRecord::Base.connection_handler.prevent_writes if ActiveRecord::Base.legacy_connection_handling
return false if owner_name.nil?

klass = self.owner_name.safe_constantize
klass&.current_preventing_writes
end

def migrations_paths # :nodoc:
Expand Down Expand Up @@ -196,6 +202,10 @@ def lease
@owner = Thread.current
end

def owner_name # :nodoc:
@pool.owner_name
end

def schema_cache
@pool.get_schema_cache(self)
end
Expand Down
Expand Up @@ -11,9 +11,7 @@ class SQLite3AdapterPreventWritesTest < ActiveRecord::SQLite3TestCase
self.use_transactional_tests = false

def setup
@conn = Base.sqlite3_connection database: ":memory:",
adapter: "sqlite3",
timeout: 100
@conn = ActiveRecord::Base.connection
end

def test_errors_when_an_insert_query_is_called_while_preventing_writes
Expand Down Expand Up @@ -100,9 +98,7 @@ def setup
@old_value = ActiveRecord::Base.legacy_connection_handling
ActiveRecord::Base.legacy_connection_handling = true

@conn = Base.sqlite3_connection database: ":memory:",
adapter: "sqlite3",
timeout: 100
@conn = ActiveRecord::Base.connection

@connection_handler = ActiveRecord::Base.connection_handler
end
Expand Down
Expand Up @@ -349,6 +349,71 @@ def test_connected_to_many
ActiveRecord::Base.establish_connection(:arunit)
ENV["RAILS_ENV"] = previous_env
end

def test_prevent_writes_can_be_changed_granularly
previous_env, ENV["RAILS_ENV"] = ENV["RAILS_ENV"], "default_env"

# replica: true is purposefully left out so we can test the pools behavior
config = {
"default_env" => {
"primary" => { "adapter" => "sqlite3", "database" => "test/db/primary.sqlite3" },
"primary_replica" => { "adapter" => "sqlite3", "database" => "test/db/primary.sqlite3" },
"secondary" => { "adapter" => "sqlite3", "database" => "test/db/secondary.sqlite3" },
"secondary_replica" => { "adapter" => "sqlite3", "database" => "test/db/secondary_replica.sqlite3" }
}
}

@prev_configs, ActiveRecord::Base.configurations = ActiveRecord::Base.configurations, config

PrimaryBase.connects_to database: { writing: :primary, reading: :primary_replica }
SecondaryBase.connects_to database: { writing: :secondary, reading: :secondary_replica }

# Switch everything to writing
ActiveRecord::Base.connected_to(role: :writing) do
assert_not_predicate ActiveRecord::Base.connection, :preventing_writes?
assert_not_predicate PrimaryBase.connection, :preventing_writes?
assert_not_predicate SecondaryBase.connection, :preventing_writes?

# Switch only primary to reading
PrimaryBase.connected_to(role: :reading) do
assert_predicate PrimaryBase.connection, :preventing_writes?
assert_not_predicate SecondaryBase.connection, :preventing_writes?

# Switch global to reading
ActiveRecord::Base.connected_to(role: :reading) do
assert_predicate PrimaryBase.connection, :preventing_writes?
assert_predicate SecondaryBase.connection, :preventing_writes?

# Switch only secondary to writing
SecondaryBase.connected_to(role: :writing) do
assert_predicate PrimaryBase.connection, :preventing_writes?
assert_not_predicate SecondaryBase.connection, :preventing_writes?
end

# Ensure restored to global reading
assert_predicate PrimaryBase.connection, :preventing_writes?
assert_predicate SecondaryBase.connection, :preventing_writes?
end

# Switch everything to writing
ActiveRecord::Base.connected_to(role: :writing) do
assert_not_predicate PrimaryBase.connection, :preventing_writes?
assert_not_predicate SecondaryBase.connection, :preventing_writes?
end

assert_predicate PrimaryBase.connection, :preventing_writes?
assert_not_predicate SecondaryBase.connection, :preventing_writes?
end

# Ensure restored to global writing
assert_not_predicate PrimaryBase.connection, :preventing_writes?
assert_not_predicate SecondaryBase.connection, :preventing_writes?
end
ensure
ActiveRecord::Base.configurations = @prev_configs
ActiveRecord::Base.establish_connection(:arunit)
ENV["RAILS_ENV"] = previous_env
end
end
end
end
Expand Down

0 comments on commit b92dc5b

Please sign in to comment.