Remove excess memoization of ConnAdapter instances #318
Conversation
- default_conn_adapter method already memoizes ConnAdapter instance if needed - fixes ConnAdapter being global despite default_conn_adapter has thread-local memoization
| # will cause it to return to AR pool, and it will become shared | ||
| # between memoized value and pool. And Rails does | ||
| # clear_active_connections! after each request | ||
| return ConnAdapter.new(ActiveRecord::Base.connection.raw_connection) |
There was a problem hiding this comment.
I think it makes sense to move this part to the top (as an early return), because if rails_connection_sharing_enabled? is true, then t = Thread.current etc lines are effectively dead.
There was a problem hiding this comment.
There's another return before this, in order for setter to work. If thread-local :qc_conn_adapter is set before with setter, it's returned regardless of rails_connection_sharing_enabled?.
This setter is a little odd (even in master), as it sets default_conn_adapter only for current thread. After this changes, this behavior is retained. I think such setters are better to be deprecated.
There was a problem hiding this comment.
I see. For a future reviewer, if we forget about setter, the proposed change in essence is:
def self.default_conn_adapter
if rails_connection_sharing_enabled?
ConnAdapter.new(ActiveRecord::Base.connection.raw_connection)
else
Thread.current[:qc_conn_adapter] ||= ConnAdapter.new
end
end
This was definitely a problem for us when we upgraded from Rails 5.1 to 5.2. Due to AR reaping an idle connection, we had hard to debug error: if a worker was idle for 300 seconds or more and then finally received a request, all AR models continued to work fine, but invoking Current workaround for us is to disable sharing connection via setting @kolen thanks for looking into this! |
|
Closing, seems that the issue has been fixed. |
Removed memoization of
ConnAdapterreturned byQC.default_conn_adapterinQueueQC.default_conn_adapteralready memoizesConnAdapterinstance if needed, so memoizing it again is unnecessary.This double memoization had problem: despite
default_conn_adaptermemoizing its connection adapter instance as thread-local variable, there's global instance ofQueuein@default_queue, and it memoizes@conn_adaptertaken fromQC.default_conn_adapter. So connection becomes global.queue_classic/lib/queue_classic/config.rb
Lines 30 to 33 in 4260d89
For example, this singleton connection causes problems with ActiveRecord pool reaper (ActiveRecord connection sharing: connection is returned to the pool #317 (comment)).
If connection adapter (and its connection) was really intended to be global, and not per-thread, then I don't understand why
QC.default_conn_adapterhas thread-local memoization (8e208e2).As
Queuehas setterconn_adapter=, for backwards compatibility, if connection is set via setter, it's then returned byconn_adapter. However, I don't understand necessity of this setter API, as overwriting connection adapter for existing queue instance is a recipe for mess. Maybe should be deprecated in future.Potential problems with this change: it breaks assumption that there's single global connection for all threads. But 8e208e2 suggests that it's not design assumption but a bug, and there should be one connection per thread. If an app starts lots of threads, uses QC in them and then kills them, it would be really bad as connections will remain open until GC, however isn't it by design? ConnAdapter has
disconnectmethod.Removed memoization of
ActiveRecord::Base.connectionindefault_conn_adapterConnection borrowed from ActiveRecord pool by calling
connectionshouldn't be stored anywhere as it will be likely returned back to pool withActiveRecord::Base.clear_active_connections!, or withreapif thread that borrowed it is dead. Rails calls this method after handling each request. Again, ActiveRecord has its own memoization for connection assigned to current thread.This probably fixes ActiveRecord connection sharing: connection is returned to the pool #317 (common
PG::ConnectionBad: connection is closederror).Memoization for non-activerecord connections remains, as connection managed by QC itself should have its owner. Also, there's setter too, and if ConnAdapter is set via setter, it's no longer taken from ActiveRecord.
Potential problems with this change: when using ActiveRecord without Rails, you have to call
clear_active_connections!, but at least now it's safe to call it.