ActiveRecord connection sharing: connection is returned to the pool

When ActiveRecord integration is enabled, ConnAdapter is constructed with connection taken from ActiveRecord pool:

Line 57 in 4260d89

ConnAdapter.new(ActiveRecord::Base.connection.raw_connection)

However, according to ActiveRecord documentation, it's not a correct way to do it, as the connection will be returned to the pool by Action Pack at the end of request:

Simply use ActiveRecord::Base.connection as with Active Record 2.1 and earlier (pre-connection-pooling). Eventually, when you're done with the connection(s) and wish it to be returned to the pool, you call ActiveRecord::Base.clear_active_connections!. This will be the default behavior for Active Record when used in conjunction with Action Pack's request handling cycle.

As ConnAdapter is not going to release its connection, ActiveRecord::Base.connection_pool.checkout should be used.

This might cause PG::ConnectionBad: connection is closed problem described here: #302 (comment), which I have regularly too. (Also there were old issues from the time ActiveRecord integration was not supported: #141, #134, #96). Pool may close connections returned to it, and it may close connection that appears to be shared between QC::ConnAdapter and the pool after that accidental connection release.

Proposed change is:

--- a/lib/queue_classic.rb
+++ b/lib/queue_classic.rb
@@ -59,7 +59,7 @@
   def self.default_conn_adapter
     return @conn_adapter if defined?(@conn_adapter) && @conn_adapter
     if rails_connection_sharing_enabled?
-      @conn_adapter = ConnAdapter.new(ActiveRecord::Base.connection.raw_connection)
+      @conn_adapter = ConnAdapter.new(ActiveRecord::Base.connection_pool.checkout.raw_connection)
     else
       @conn_adapter = ConnAdapter.new
     end

However, I don't know yet how to make reproducible test case, as it involves pool state.

Activerecord pool may close connections in e.g. flush.

Closed connection state is distinct from just disconnected (i.e. by TCP connection reset). Closed connection can't be reconnected by reset method used by ConnAdapter:

> conn.close
=> nil
> conn.exec "select 1 from ads;"
PG::ConnectionBad: connection is closed
> conn.reset
PG::ConnectionBad: connection is closed

That's why once this error happens, it persists for the lifetime of ConnAdapter (and therefore of queue).

Note that it only causes problems with QC.enqueue from Rails web app. It does not cause problems with worker, as long as tasks don't do ActiveRecord::Base.clear_active_connections!.

Seems that just replacing ActiveRecord::Base.connection.raw_connection with ActiveRecord::Base.connection_pool.checkout.raw_connection, as I proposed, will not work reliably: ActiveRecord has "reaping" feature that returns checked out connections to pool.

It decides to recover connection back if thread that checked out the connection is no longer alive. In queue_classic default queue's connection is effectively global:

default_conn_adapter is thread-local memoized value:

queue_classic/lib/queue_classic.rb

Lines 53 to 54 in 4260d89

def self.default_conn_adapter

t = Thread.current
but default_queue is global:

queue_classic/lib/queue_classic/config.rb

Lines 31 to 33 in 4260d89

def default_queue

@default_queue ||= Queue.new(QC.queue)

end
and Queue instance memoizes connection adapter (and connection adapter holds connection):

queue_classic/lib/queue_classic/queue.rb

Lines 21 to 23 in 4260d89

def conn_adapter

@adapter ||= QC.default_conn_adapter

end

So, when AR pool reaps connections, if thread in which QC was touched for the first time is dead, the connection will be stolen from ConnAdapter and will become shared with AR pool.

Probably better way to fix this would be to leave ActiveRecord::Base.connection.raw_connection untouched, but to remove memoization of @conn_adapter in Queue. However, it also has setter conn_adapter=, and keeping API compatibility will be trickier.

This will work for non-activerecord connections too, as conn adapter is already memoized in thread-local in QC.default_conn_adapter: no need to memoize it further in Queue.

(However, for ActiveRecord connections, ActiveRecord::Base.connection.raw_connection should never be memoized)

	def default_queue
	@default_queue \|\|= Queue.new(QC.queue)
	end

	def conn_adapter
	@adapter \|\|= QC.default_conn_adapter
	end