4
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

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.

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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