Monitoring and re-establishing a PostgreSQL connection

Question

Within a complicated Ruby project, I need to monitor the connection to a PostgreSQL database, as answered here. I use a thread for this purpose. Every few milliseconds, I invoke "consume_input" which will raise an exception of the database connection no longer exists. If might have been closed for a number of reasons, but there is no indication from the socket/connection why it is closed. If the server is restarted and is alive again within a certain time-frame, I reconnect and assume everything is fine. Otherwise, I notify another object via a callback that all is not well, and the thread exits normally.

I'm looking for tips and suggestions on following best practices and correctness. I need some general Ruby advice on making my code cleaner and more robust.

PG_MAX_FAILURES=10
PG_SLEEP_QUANTA=0.25
def monitor_start
 log.info("db: #{__method__}")
 _dbconn=nil
 if @monitor.alive?
 @monitor.kill
 sleep PG_SLEEP_QUANTA
 end
 @monitor = Thread.new {
 _failures=0
 while true do
 begin
 if ! _dbconn.is_a?PG::Connection or _dbconn.status() != PG::CONNECTION_OK
 log.info("Reconnecting...")
 _dbconn = PG.connect( @dbconnstr )
 log.info("Reconnected.")
 else
 _dbconn.consume_input()
 _failures=0
 end
 sleep PG_SLEEP_QUANTA
 rescue PG::ConnectionBad => ex
 _failures += 1
 if _failures >= PG_MAX_FAILURES
 @monitor_post_callback.call()
 break # Thread.current.stop ??
 end
 log.warn("Postgresql connection lost. Retry# #{_failures}...")
 sleep PG_SLEEP_QUANTA
 retry
 rescue => ex
 log.warn("Other exception #{ex.class} #{ex.message}")
 end
 end
 }
end

Answer 1

What follows is just my subjective opinion; you can disagree.

1. Robust code is maintainable. You need to be able to change this code if you find a bug in it, or if the requirements change. It might even be that someone else may need to maintain this code. So you should probably use a regular indenting pattern (2 spaces is normal for Ruby). That would make it a lot easier to see what was going on.

2. Likewise, your code would be cleaner if it was split into separate functions. Making the contents of the thread.new block a separate function would be a good place to start, but you could (and maybe should) go further than that.

3. It seems to me that the sleep ... retry at the end of the first rescue block are redundant, since you are going to loop around again anyway. YMMV.

• Otheus: If the read fails, there is no other point where sleep is called. But I see your point that retry is technically redundant. Two-space indentation noted, thank you.
• Otheus: Can't split into two functions here. The code preceding the Thread is to make sure that multiple instances of the thread aren't run concurrently.
• Andy Jones: @monitor = Thread.new { thread_body_function } ?
• Otheus: Hrm, that might make unit-testing easier.
• Andy Jones: Exactly. The more you break the code up, the easier it is to read (and therefore maintain) and the easier it is to test. And if you pass _dbconn into thread_body_function it gets even better: dependancy injection makes the tests even easier (mocks & stubs) plus it makes it easier to break the code up further.