Implement ActiveRecord.disconnect_all! to close all connections

[casperisfine]

This is basically a multi-db aware version of ActiveRecord::Base.connection.disconnect!. It also avoid connecting to the database if we weren't already.

This can be useful to reset state after establish_connection has been used.

[eileencodes]

My concern with this being ActiveRecord.disconnect! defined at the top level is the API is confusing. Disconnect what? Whereas ActiveRecord.connection.disconnect! is clearly disconnecting the connection. I think this should be in the connection handler like: ActiveRecord::Base.connection_handler.disconnect!. What do you think?

[casperisfine]

Whereas ActiveRecord.connection.disconnect! is clearly disconnecting the connection.

Yeah, but it's also connecting first if you weren't already, which is annoyinh.

What do you think?

What about disconnect_all!? The usage is really to ask AR to close all its connections ActiveRecord::Base.connection_handler.disconnect! is much less clear whether it will close other pools etc.

[casperisfine]

Hum, these failures are WTF:

TestUnconnectedAdapter#test_underlying_adapter_no_longer_active:
NoMethodError: undefined method `synchronize' for :async:Symbol
 
    @_mutex.synchronize(&block)
           ^^^^^^^^^^^^
    /usr/local/lib/ruby/3.1.0/mutex_m.rb:79:in `mu_synchronize'

Error:
TestUnconnectedAdapter#test_connection_no_longer_established:
NoMethodError: undefined method `each' for #<Arel::Nodes::SelectCore:...>
    /rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:308:in `block in discard!'
    /usr/local/lib/ruby/3.0.0/monitor.rb:202:in `synchronize'
    /usr/local/lib/ruby/3.0.0/monitor.rb:202:in `mon_synchronize'

It's like we're triggering a GC bug or something.

[matthewd]

I really don't want people doing this, and I don't understand why it's necessary.

[byroot]

I really don't want people doing this

May I ask why? I also don't want this to be required, and it's not, but it doesn't hurt.

I don't understand why it's necessary.

You can let Rails abandon connections yes, but eagerly closing them when you can is cleaner as you won't have to wait for a timeout or a keepalive for the server to realize the connection was closed.

[matthewd]

The forking process can continue to use those connections. When that process terminates, or loses interest in them (including via AR's idle timeout), then it will proactively disconnect them.

#31241 was the culmination of a gradual path of eliminating the need for superstitious ritual around forking, and I really don't want to bring it back. "It's preferable" sounds like exactly that, as does "ideally applications should". Every server config having some variant spelling of disconnect-before and/or establish-after is harmful: to perceived reliability, reasoned discussion of expected behaviour (where party B comes in and points out that Party A is using Ritual X, but they've heard Ritual Y is better, so that's probably the problem), and to performance and application behaviour if you're repeatedly dropping connections that the parent process is actually using.

I can believe a very specific use case for this behaviour, but it's not "before fork" -- it's "when deciding to repurpose an existing process to do a completely different job".

I don't object to the existence of a "disconnect everything, empty out all the pools, but keep them available" method (the code change is fine) -- but the PR description, branch name, and method documentation do not reflect the extremely specialist use I foresee.

[byroot]

Right, I think I need to explain why I came to this because indeed it's not clear.

The use case here isn't for forking in the middle of a request or job, it's for Pitchfork promotion.

In this use case it's not closing connections that the parent is actively using, but a worker basically getting out of rotation and not needing requests anymore. Not doing anything works thanks to the automatic discard on fork, but it's a bit wasteful and I'd like a handy method to drop all connections safely.

With Puma or Unicorn, it's not a huge deal because you only got one process (the parent) keeping useless connections, but with Pitchfork, if you spawn new generations fast enough, dropped connections can pile up on the database server, and that can be a problem, making this eager disconnect more important.

I can believe a very specific use case for this behaviour, but it's not "before fork" -- it's "when deciding to repurpose an existing process to do a completely different job".

Yup, that pretty much describe Pitchfork promotion process.

[matthewd]

Yeah that paragraph was "I know you want this for Pitchfork" 😄

I'm still a bit lost on how connections are piling up, though you're obviously seeing it happen. It doesn't sound like something I'd expect to happen, and so I'd still like to further explore it and understand why we can't account for it internally before turning it into a public API for you to use here.

I can be convinced that the API should exist. Convincing me it should be documented with a spooky "if you don't call this, things might work, they might not, who knows?" description is a much taller order.

[casperisfine]

I'm still a bit lost on how connections are piling up

So I haven't tested all systems, but you can somewhat simulate it with MySQL:

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'activerecord'
  gem 'mysql2'
end

require 'active_record'

10.times do
  ActiveRecord::Base.establish_connection(adapter: "mysql2", username: "root", database: "activerecord_unittest")
  ActiveRecord::Base.connection.execute("SELECT 1");
  ActiveRecord::ConnectionAdapters::PoolConfig.discard_pools!
end

ActiveRecord::Base.establish_connection(adapter: "mysql2", username: "root", database: "activerecord_unittest")
puts ActiveRecord::Base.connection.execute("show status where `variable_name` = 'Threads_connected'").to_a

Threads_connected
11

Overall, I don't think it's a huge deal for MySQL as connections are cheap there, and they don't pile up forever, but I'm worried about Postgres (maybe unnecessarilly so?) because I know connections are much more pricy there (but maybe PG bouncer can be assumed now-adays?).

Convincing me it should be documented with a spooky "if you don't call this, things might work, they might not, who knows?" description is a much taller order.

Yeah, that definitely not my intent. What I want to convey is that if you know this process won't ever use the DB anymore, you can call that to reclaim resources eagerly. If you forget, no big deal, you likely have enough leeway on your database server to wait a bit for the connections to be dropped by the server.

[matthewd]

That's not forking though.

My claim is that we only discard, during fork, because another process still owns the connection. If you discard something that's not owned by some other process, then yeah you're dropping things on the ground and the server will have no idea what's up... but Active Record isn't supposed to ever be doing that.

When that original, forking, owning process -- which never did a discard -- terminates, the connection should get properly cleaned up.

This does mean that (without an explicit disconnect, up until the connections hit their idle timeout) a previously-active worker process that's been directly promoted to a mold would hold a set of connections while it still exists. But as soon as that process terminates, they should go away, so there'd be no ongoing accumulation.

And it's my loose understanding that pitchfork doesn't actually directly promote workers to molds in-place, but that the candidate worker (double?) forks a new mold process... I haven't checked whether the worker then continues working or terminates, but either 1) that original worker should continue using its existing connections, or 2) when that worker stops, it will properly shut them down.

[byroot]

so there'd be no ongoing accumulation.

It's not totally unbounded yes. However it means newly forked worker start with a bunch of IO objects that need to be garbage collected. Again, it's not functionally broken, but I'd like the option to eagerly close them.

pitchfork doesn't actually directly promote workers to molds in-place, but that the candidate worker (double?) forks a new mold process...

No it's in place, the worker process just change role. An after_promotion callback is invoked, and that's where ideally connections would be eagerly closed.

[matthewd]

Oh, I mis-skimmed https://github.com/Shopify/pitchfork/blob/master/docs/REFORKING.md#forking-sibling-processes as being about forking of the mold process, but yeah I now see what's happening there.

it means newly forked worker start with a bunch of IO objects

I mean, the adapter objects and surrounding ivars will dwarf the actual IO objects, but the point remains.

No it's in place, the worker process just change role. An after_promotion callback is invoked, and that's where ideally connections would be eagerly closed.

Yeah, that sounds totally reasonable and sensible to me. But it has nothing to do with forking (and absolutely nothing to do with fork leaving connections hanging around the server)... it is "we're repurposing this process, so whatever it was doing before is no longer relevant". Which is a somewhat obscure-feeling thing to do / accomodate... and does leave me wondering if it'd be neater and more universal [given that your running app is by-definition fork-friendly] to fork a mold, but still, fine.

[casperisfine]

Ok, so I've changed Pitchfork so that it now fork molds from workers, so this isn't nearly as much of a concern now, as connections will be discarded and likely GCed rather quickly.

I still think such API would be useful to have in general, but I agree the doc might need to be clearer on what the use case are.

[casperisfine]

We discussed this in person with @matthewd. We agreed the feature is fine, we should just not talk about forking etc, as to not mislead users in its necessity.

I updated the documentation and renamed the method to ActiveRecord.disconnect_all! for clarity.

[Ivanov-Anton]

Great job! @casperisfine