Makara Proxy breaks the relationship between ActiveRecord ConnectionPool and ConnectionAdapter

[jeffdoering]

(Note: The following analysis is based on Rails 5.2/6; I suspect the issues go back to older versions but did not check details. ActiveRecord ConnectionPooling has definitely evolved in recent versions so some of this may not apply to past versions.)

Makara's Proxy pattern fails to fully control the relationship between an ActiveRecord ConnectionPool and the ConnectionAdapters (aka connections) it contains. The MakaraAbstractAdapter is intended to fully encapsulate the underlying ActiveRecord AbstractAdapters it uses internally for both its slave and master connection lists. The "real" connections are held by the MakaraAbstractAdapter which delegates to them depending on the situations (reads to slaves, writes to masters, replacing bad connections, etc). To accomplish this intelligent delegation Makara uses a combination of:

• dynamically generating specific delegators methods in it's hijack_method logic
• sending some known methods to all connections with send_to_all
• sending some known methods to super.method_missing
• intercepting all other methods via method_missing and sending them to a semi-arbitrary real connection via any_connection.

The primary problem is with the final method_missing catch-all approach. Makara's Proxy is not explicitly aware of several important connection management methods ActiveRecord uses. Here's a quick sample of ActiveRecord AbstractAdapter methods that Makara does not special-case:

    def close; end
    def steal!; end
    def expire; end
    def lease; end
    def in_use?; end
    def active?; end
    def owner; end
    def pool; end
    def pool=(p);
    def lock; end
    def schema_cache=(cache); end
    def _run_checkout_callbacks; end
    def _run_checkin_callbacks; end
    def verify!; end

Most ActiveRecord ConnectionAdapters (e.g. PostgreSQLAdapter) directly subclass ActiveRecord::ConnectionAdapters::AbstractAdapter and inherit the default behavior for these methods. For example pool= is used by ActiveRecord to tell a ConnectionAdapter about it's ConnectionPool. Close is implemented as follows:

     def close
        pool.checkin self
      end

Unfortunately because Makara does not special-case pool=; it ends up setting up a relationship between the ConnectionPool and one of real ConnectionAdapters it is managing - typically one of the @master_pool connections of the proxy; but it could also be a @slave_pool connection if the master connections are all blacklisted. The other connections don't get a pool set at all. Later if an application calls connection.close it might raise an error because pool is nil or it might actually put the real connection into the ConnectionPool's list of free connections - which poisons the pool by leaking an underlying adapter into a pool that should be all MakaraAdapters. There are other cases around missing owner= calls and connection management that can raise errors for ActiveRecord code that is trying to manage connections in it's pools.

Finally; because ActiveRecord uses the methods above for pool management - many of them can be called from threads that do not own the connection. This exposes the MakaraAdapter's underlying ConnectionWrapper arrays to concurrency when any_connection traverses them. These arrays are not protected by locking. In comparison to the connection/pool state management issues above the threading problem is harder to quantify. It might cause problems but many problems don't require concurrency at all - just blacklisting of connection so that the state of a real connection that receives one call is wrong because it did not receive some earlier call.

The most obvious (but complex) solution to this problem would be for Makara's Proxy to intercept the calls above intelligently and make sure that the real connections only got references to a fake Makara provided ConnectionPool reference and that the Proxy itself answered the method calls for things like in_use?, owner, etc itself. However; without subclasses ActiveRecord AbstractAdapter - keeping track of any new methods used for pool management appears to be a maintenance concern with an approach that requires explicitly whitelisting methods that need special handling.