Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveRecord connection sharing: connection is returned to the pool #317

Open
kolen opened this issue Aug 9, 2019 · 4 comments
Open

ActiveRecord connection sharing: connection is returned to the pool #317

kolen opened this issue Aug 9, 2019 · 4 comments

Comments

@kolen
Copy link

kolen commented Aug 9, 2019

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

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.

@kolen
Copy link
Author

kolen commented Aug 9, 2019

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).

@kolen
Copy link
Author

kolen commented Aug 9, 2019

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!.

@kolen
Copy link
Author

kolen commented Aug 10, 2019

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:

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.

@kolen
Copy link
Author

kolen commented Aug 10, 2019

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)

@kolen kolen changed the title ActiveRecord connection sharing: connection is returned to the pool by Action Pack ActiveRecord connection sharing: connection is returned to the pool Aug 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant