fix: define connection holder more thread-safe

[jughead]

AR's establish_connection is not thread-safe. If standby connection is not established beforehand concurrent threads will raise something like:

ActiveRecord::ConnectionNotEstablished: No connection pool with id SlaverySlaveConnectionHolder found.

[jughead]

Without the change don't know exactly why, but with sqlite3 the test passes, with "real" db (postgresql in the example) test fails. Couldn't get to the fail scenario with sqlite3. Would be good to update CI to include pg.

The actual issue is not connected to any specific DB adapter, the issues comes from the fact that establish_connection calls connection_handler.remove_connection and then connection_handler.establish_connection, which in turn remove and set the connection pool in the Concurrent::Map (however it doesn't matter). But as it happens concurrently when the query is issued in one of the threads, connection_pool might not be there in the connection_handler's map.

[kenn]

Hi @jughead - I think introducing Mutex for this is overkill, wouldn't it be simpler to add

Standby.on_standby { ActiveRecord::Base.connection.execute('SELECT 1') }

In after_fork of puma/unicorn, where you'd usually put ActiveRecord::Base.establish_connection for the primary connection anyway. Would that solve the problem for you?

[jughead]

Hi @kenn! Thank you for quick reply!

It happens in Sidekiq actually, so I need to add it there before thread spawning. It helps, but generally everyone need to remember to establish_connection beforehand.

In your suggested approach, you still need to be sure that the connection is returned back in the main thread. Standby.on_standby block doesn't check in the connection back to the connection pool, as it relies on the fact that it's wrapped in some Rails middleware (Rails.application.executor.wrap that automatically cleans up connection at the end of processing (being it request, job, or something else)), but if we put the connection initialization before thread spawning, we must checkin the connection by ourselves. Correct me if I'm wrong.

If you insist on the calling Standby beforehand, I think it's better to put this in readme then.

[jughead]

Just to be clear, mutex is used only once per standby during the initilization. In all other cases, it's not used, as the hash is filled.