Skip to content

Commit 0396fe9

Browse files
committed
Refactor connection handler's establish_connection
In the prior code we were getting the pool manager a bunch of times - once in the retrieve_connection_pool, once in remove_connection_pool, and once in the else conditional after setting it. This change ensures we're only getting the pool manager once. If there is no pool manager for the given key then we create a new one in the new `set_pool_manager` method and use that. The refactor also has a change in that we no longer instrument if a new connection was not established. This instrumentation is private though (denoted by the !) so it's safe to make this change. Followup to rails#45450
1 parent 462545c commit 0396fe9

File tree

1 file changed

+30
-21
lines changed

1 file changed

+30
-21
lines changed

activerecord/lib/active_record/connection_adapters/abstract/connection_handler.rb

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -106,29 +106,29 @@ def establish_connection(config, owner_name: Base, role: ActiveRecord::Base.curr
106106
pool_config = resolve_pool_config(config, owner_name, role, shard)
107107
db_config = pool_config.db_config
108108

109+
pool_manager = set_pool_manager(pool_config.connection_specification_name)
110+
109111
# If there is an existing pool with the same values as the pool_config
110112
# don't remove the connection. Connections should only be removed if we are
111113
# establishing a connection on a class that is already connected to a different
112114
# configuration.
113-
pool = retrieve_connection_pool(pool_config.connection_specification_name, role: role, shard: shard)
114-
115-
unless pool && pool.db_config == db_config
116-
remove_connection_pool(pool_config.connection_specification_name, role: role, shard: shard)
115+
existing_pool_config = pool_manager.get_pool_config(role, shard)
117116

118-
owner_to_pool_manager[pool_config.connection_specification_name] ||= PoolManager.new
119-
pool_manager = get_pool_manager(pool_config.connection_specification_name)
117+
if existing_pool_config && existing_pool_config.db_config == db_config
118+
existing_pool_config.pool
119+
else
120+
disconnect_pool_from_pool_manager(pool_manager, role, shard)
120121
pool_manager.set_pool_config(role, shard, pool_config)
121-
pool = pool_config.pool
122-
end
123122

124-
payload = {
125-
spec_name: pool_config.connection_specification_name,
126-
shard: shard,
127-
config: db_config.configuration_hash
128-
}
123+
payload = {
124+
spec_name: pool_config.connection_specification_name,
125+
shard: shard,
126+
config: db_config.configuration_hash
127+
}
129128

130-
ActiveSupport::Notifications.instrumenter.instrument("!connection.active_record", payload) do
131-
pool
129+
ActiveSupport::Notifications.instrumenter.instrument("!connection.active_record", payload) do
130+
pool_config.pool
131+
end
132132
end
133133
end
134134

@@ -196,12 +196,7 @@ def connected?(spec_name, role: ActiveRecord::Base.current_role, shard: ActiveRe
196196

197197
def remove_connection_pool(owner, role: ActiveRecord::Base.current_role, shard: ActiveRecord::Base.current_shard)
198198
if pool_manager = get_pool_manager(owner)
199-
pool_config = pool_manager.remove_pool_config(role, shard)
200-
201-
if pool_config
202-
pool_config.disconnect!
203-
pool_config.db_config
204-
end
199+
disconnect_pool_from_pool_manager(pool_manager, role, shard)
205200
end
206201
end
207202

@@ -221,6 +216,20 @@ def get_pool_manager(owner)
221216
owner_to_pool_manager[owner]
222217
end
223218

219+
# Get the existing pool manager or initialize and assign a new one.
220+
def set_pool_manager(owner)
221+
owner_to_pool_manager[owner] ||= PoolManager.new
222+
end
223+
224+
def disconnect_pool_from_pool_manager(pool_manager, role, shard)
225+
pool_config = pool_manager.remove_pool_config(role, shard)
226+
227+
if pool_config
228+
pool_config.disconnect!
229+
pool_config.db_config
230+
end
231+
end
232+
224233
# Returns an instance of PoolConfig for a given adapter.
225234
# Accepts a hash one layer deep that contains all connection information.
226235
#

0 commit comments

Comments
 (0)