Better exception handling/logging

.gitignore

@@ -1 +1,3 @@
+Gemfile.lock
 .DS_Store
+*.swp

Gemfile

@@ -0,0 +1,4 @@
+source :rubygems
+
+gem 'mocha', '~> 0.9.12'
+gem 'system_timer', '~> 1.0'

lib/fetcher/base.rb

@@ -1,5 +1,9 @@
+require 'logger'
+
 module Fetcher
   class Base
+    attr_reader :connection
+
     # Options:
     # * <tt>:server</tt> - Server to connect to.
     # * <tt>:username</tt> - Username to use when connecting to server.
@@ -23,6 +27,7 @@ def initialize(options={})
         end
 
         instance_eval("@#{opt} = options[:#{opt}]")
+        @logger = create_logger(options[:logger])
       end
     end
 
@@ -59,5 +64,27 @@ def process_message(message)
     def handle_bogus_message(message) #:nodoc:
       raise NotImplementedError, "This method should be overridden by subclass"
     end
+
+    def handle_exception(ex)
+      log "Fetcher: Exception: #{ex.message}"
+      log ex.backtrace.join("\n")
+    end
+
+    def log(msg)
+      @logger.info(msg) 
+    end
+
+    def create_logger(provided_logger)
+      @logger = provided_logger || rails_logger || default_logger
+    end
+
+    def rails_logger
+      defined?(Rails) && Rails.logger
+    end
+
+    def default_logger
+      ::Logger.new(STDERR)
+    end
+
   end
 end

lib/fetcher/imap.rb

@@ -3,7 +3,6 @@
 
 module Fetcher
   class Imap < Base
-
     PORT = 143
 
     protected
@@ -28,61 +27,62 @@ def initialize(options={})
 
     # Open connection and login to server
     def establish_connection
-      timeout_call = (RUBY_VERSION < '1.9.0') ? "SystemTimer.timeout_after(15.seconds) do" : "Timeout::timeout(15) do"
+      timeout_call = (RUBY_VERSION < '1.9.0') ? "SystemTimer.timeout_after(15) do" : "Timeout::timeout(15) do"
 
       eval("#{timeout_call}
-              @connection = Net::IMAP.new(@server, @port, @ssl)
+              connection = Net::IMAP.new(@server, @port, @ssl)
               if @use_login
-                @connection.login(@username, @password)
+                connection.login(@username, @password)
               else
-                @connection.authenticate(@authentication, @username, @password)
+                connection.authenticate(@authentication, @username, @password)
               end
             end")
     end
 
     # Retrieve messages from server
     def get_messages
-      @connection.select(@in_folder)
-      @connection.uid_search(['ALL']).each do |uid|
-        msg = @connection.uid_fetch(uid,'RFC822').first.attr['RFC822']
+      connection.select(@in_folder)
+      connection.uid_search(['ALL']).each do |uid|
+        msg = connection.uid_fetch(uid,'RFC822').first.attr['RFC822']
         begin
           process_message(msg)
           add_to_processed_folder(uid) if @processed_folder
-        rescue
+        rescue Exception => ex
+          handle_exception(ex)
           handle_bogus_message(msg)
         end
         # Mark message as deleted 
-        @connection.uid_store(uid, "+FLAGS", [:Seen, :Deleted])
+        connection.uid_store(uid, "+FLAGS", [:Seen, :Deleted])
       end
     end
 
     # Store the message for inspection if the receiver errors
     def handle_bogus_message(message)
       create_mailbox(@error_folder)
-      @connection.append(@error_folder, message)
+      connection.append(@error_folder, message)
     end
 
     # Delete messages and log out
     def close_connection
-      @connection.expunge
-      @connection.logout
+      connection.expunge
+      connection.logout
       begin
-        @connection.disconnect unless @connection.disconnected?
+        connection.disconnect unless connection.disconnected?
       rescue
-        Rails.logger.info("Fetcher: Remote closed connection before I could disconnect.")
+        log("Fetcher: Remote closed connection before I could disconnect.")
       end
     end
 
     def add_to_processed_folder(uid)
       create_mailbox(@processed_folder)
-      @connection.uid_copy(uid, @processed_folder)
+      connection.uid_copy(uid, @processed_folder)
     end
 
     def create_mailbox(mailbox)
-      unless @connection.list("", mailbox)
-        @connection.create(mailbox)
+      unless connection.list("", mailbox)
+        connection.create(mailbox)
       end
     end
-    
+
   end
 end

lib/fetcher/pop.rb

@@ -2,7 +2,6 @@
 
 module Fetcher
   class Pop < Base
-
     protected
 
     # Additional Options:
@@ -16,18 +15,19 @@ def initialize(options={})
 
     # Open connection and login to server
     def establish_connection
-      @connection = Net::POP3.new(@server, @port)
-      @connection.enable_ssl(OpenSSL::SSL::VERIFY_NONE) if @ssl
-      @connection.start(@username, @password)
+      connection = Net::POP3.new(@server, @port)
+      connection.enable_ssl(OpenSSL::SSL::VERIFY_NONE) if @ssl
+      connection.start(@username, @password)
     end
 
     # Retrieve messages from server
     def get_messages
-      unless @connection.mails.empty?
-        @connection.each_mail do |msg|
+      unless connection.mails.empty?
+        connection.each_mail do |msg|
           begin
             process_message(msg.pop)
-          rescue
+          rescue Exception => ex
+            handle_exception(ex)
             handle_bogus_message(msg.pop)
           end
           # Delete message from server
@@ -40,10 +40,10 @@ def get_messages
     def handle_bogus_message(message)
       # This needs a good solution
     end
-    
+
     # Close connection to server
     def close_connection
-      @connection.finish
+      connection.finish
     end
 
   end

test/fetcher_test.rb

@@ -38,6 +38,34 @@ def test_should_require_password
   def test_should_require_receiver
     assert_raise(ArgumentError) { create_fetcher(:receiver => nil) }
   end
+
+  def test_should_handle_exception_if_receiver_raises_exception_during_receive
+    @imap_fetcher = Fetcher.create(:type => :imap, :server => 'test.host',
+                                   :username => 'name',
+                                   :password => 'password',
+                                   :receiver => @receiver)
+
+    # most of this stubbing/mocking is to prevent the fetcher from 
+    # actually connecting to a server.
+    @fake_message = stub('fake-message')
+    @stub_connection = stub_everything('fake-imap-connection', {
+      :uid_search => ['123'], 
+      :uid_fetch  => [stub('fake-message-piece', :attr => {"RFC822" => @fake_message})]
+    })
+    @imap_fetcher.stubs(:establish_connection).returns(true)
+    @imap_fetcher.stubs(:connection).returns(@stub_connection)
+
+    # The meat of this test
+    exception = RuntimeError.new("Explosion")
+    @receiver.expects(:receive).with(@fake_message).raises(exception)
+
+    # we should handle the exception
+    @imap_fetcher.expects(:handle_exception).with(exception)
+
+    # but also ensure we handle the bogus message as well
+    @imap_fetcher.expects(:handle_bogus_message).with(@fake_message)
+    @imap_fetcher.fetch
+  end
 
   def create_fetcher(options={})
     @fetcher = Fetcher::Base.new({:server => 'test.host', :username => 'name', :password => 'password', :receiver => @receiver}.merge(options))
@@ -71,4 +99,4 @@ def test_should_require_type
 
 end
 
-# Write tests for sub-classes
+# Write tests for sub-classes