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.

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

1 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.

  • \$\begingroup\$ 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. \$\endgroup\$
    – Otheus
    Commented May 29, 2015 at 12:27
  • \$\begingroup\$ 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. \$\endgroup\$
    – Otheus
    Commented Jun 3, 2015 at 8:19
  • \$\begingroup\$ @monitor = Thread.new { thread_body_function } ? \$\endgroup\$
    – Andy Jones
    Commented Jun 3, 2015 at 12:54
  • \$\begingroup\$ Hrm, that might make unit-testing easier. \$\endgroup\$
    – Otheus
    Commented Jun 3, 2015 at 15:16
  • \$\begingroup\$ 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. \$\endgroup\$
    – Andy Jones
    Commented Jun 4, 2015 at 7:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.