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:
- We did not realise that
establish_connectionwas being repeatedly called.
- We “knew” that even if it were, that this could only create an efficiency problem.
#establish_connectioncreates a nice, self-contained piece of machinery called the
ConnectionPooland you can use it where appropriate. Sure, it’s also memoized in
ActiveRecord::Base.connection_poolbut 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
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
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?