Suggested exception: ConnectionPoolRemoved

rubyonrails-core
1

Hello rails peoples,

Last week I solved what turned out to be the hardest bug of my career. A few of us on the team spent many person-days trying to figure out a problem in an application written by a guy who left the company shortly before it was due to be finished for launch. We spent a long time on red herrings regarding raw_connection, forking, reviewing all our explicit thread orchestrating code, deployment order of concurrent processes and many others. The application uses AR to manage and execute selective bidirectional syncing between an arbitrary number of postgres databases (amongst other things). Yes, I’m aware this was a veeeery ambitious technical choice.

I believe if the error message accompanying ConnectionNotEstablished had pointed us to one particular fact we would have fixed the problem in under an hour and I think the mistake our app was making is an easy one if you’re taking your first steps into exotic use of dynamically specified connection strings.

Our mistakes were these:

  1. We did not realise that establish_connection was being repeatedly called.
  2. We “knew” that even if it were, that this could only create an efficiency problem. #establish_connection creates a nice, self-contained piece of machinery called the ConnectionPool and you can use it where appropriate. Sure, it’s also memoized in ActiveRecord::Base.connection_pool but that’s just passively there for if you’re going for the convention rather than configuration.

Of course this is all wrong and AR is so active in the management of pools that it disconnects the previously made pool every time you call #establish_connection. This is something I found mentioned nowhere online except upon post-mortem in the AR source code

I’ve taken a look and I think some kind of warning when you call establish_connection could be possible. However, I concede it may be too late and too noisy for codebases that are tolerating the practice.

So we can’t categorically say that recalling this method is a mistake. What we can say is that calling connection on a pool which has already been deliberately closed proves that you captured the returned pool from establish_connection and as such there’s a thing you’ve not realised and some documentation to go read.

I’d suggest that if a user has no established connection because they misused establish_connection this way then instead of ConnectionNotEstablished, it’s ConnectionPoolRemoved. It will say that you disconnected this pool + link to documentation explaining how you most likely did that and explain more about the new connected_to features.

Of course, you can’t write an error class for every kind of mistake a user could make but I sense that there aren’t many other AR methods intended for wide public use whose return value should not be stored. connection is the only other one I know and the internet is full of warnings about not self -managing these.

I was in the process of writing the patch myself to implement this but I saw that you prefer functionality changes be proposed here first. Would you be happy to entertain a small patch like this with accompanying documentation update?

Cheers,

Adam

3 Likes
2

TLDR; The internet is full of warnings about the pitfalls of attempting your own DB connection management instead of leaving to AR. As multi-dbs become more of a supported case lets also warn people about the layer above: handling your own connection pools.