From: wmorgan Date: Sun, 1 Apr 2007 20:22:38 +0000 (+0000) Subject: improved source error handling X-Git-Url: https://git.notmuchmail.org/git?a=commitdiff_plain;h=5ca2ea2e96a2603825c21be3ece66f1fb719f3fe;p=sup improved source error handling git-svn-id: svn://rubyforge.org/var/svn/sup/trunk@357 5c8cc53c-5e98-4d25-b20a-d8db53a31250 --- diff --git a/bin/sup b/bin/sup index 00f0b3d..dc1f2f9 100644 --- a/bin/sup +++ b/bin/sup @@ -105,6 +105,11 @@ begin bm.draw_screen + begin + Index.usual_sources.each { |s| s.check } + rescue SourceError + # do nothing! we'll report it at the next step + end Redwood::report_broken_sources Index.usual_sources.each do |s| diff --git a/lib/sup.rb b/lib/sup.rb index cf46b8d..9be9a9e 100644 --- a/lib/sup.rb +++ b/lib/sup.rb @@ -98,21 +98,44 @@ module Redwood ## not really a good place for this, so I'll just dump it here. def report_broken_sources return unless BufferManager.instantiated? - broken_sources = Index.usual_sources.select { |s| s.broken? } + + broken_sources = Index.usual_sources.select { |s| s.error.is_a? FatalSourceError } unless broken_sources.empty? - BufferManager.spawn "Out-of-sync soure notification", TextMode.new(< true # couldn't find the most recent email + start = ids.index(cur_offset || start_offset) or raise OutOfSyncSourceError, "Unknown message id #{cur_offset || start_offset}." start.upto(ids.length - 1) do |i| id = ids[i] @@ -197,26 +197,6 @@ private @say_id = nil end - def die_from e, opts={} - @imap = nil - - message = - case e - when Exception - "Error while #{opts[:while]}: #{e.message.chomp} (#{e.class.name})." - when String - e - end - - message += " It is likely that messages have been deleted from this IMAP mailbox. Please run sup-sync --changed #{to_s} to correct this problem." if opts[:suggest_rebuild] - - self.broken_msg = message - Redwood::log message - BufferManager.flash "Error communicating with IMAP server. See log for details." if BufferManager.instantiated? - raise SourceError, message - end - - ## build a fake unique id def make_id imap_stuff # use 7 digits for the size. why 7? seems nice. msize, mdate = imap_stuff.attr['RFC822.SIZE'] % 10000000, Time.parse(imap_stuff.attr["INTERNALDATE"]) @@ -224,13 +204,12 @@ private end def get_imap_fields id, *fields - raise SourceError, broken_msg if broken? - imap_id = @imap_ids[id] or die_from "Unknown message id #{id}.", :suggest_rebuild => true + imap_id = @imap_ids[id] or raise OutOfSyncSourceError, "Unknown message id #{id}" retried = false results = safely { @imap.fetch imap_id, (fields + ['RFC822.SIZE', 'INTERNALDATE']).uniq }.first got_id = make_id results - die_from "IMAP message mismatch: requested #{id}, got #{got_id}.", :suggest_rebuild => true unless got_id == id + raise OutOfSyncSourceError, "IMAP message mismatch: requested #{id}, got #{got_id}." unless got_id == id fields.map { |f| results.attr[f] } end @@ -252,7 +231,7 @@ private raise end rescue Net, SocketError, Net::IMAP::Error, SystemCallError => e - die_from e, :while => "communicating with IMAP server" + raise FatalSourceError, "While communicating with IMAP server: #{e.message}" end end diff --git a/lib/sup/index.rb b/lib/sup/index.rb index 5d1a7b3..18efe86 100644 --- a/lib/sup/index.rb +++ b/lib/sup/index.rb @@ -295,7 +295,8 @@ protected end def load_sources fn=Redwood::SOURCE_FN - @sources = Hash[*(Redwood::load_yaml_obj(fn) || []).map { |s| [s.id, s] }.flatten] + source_array = (Redwood::load_yaml_obj(fn) || []).map { |o| Recoverable.new o } + @sources = Hash[*(source_array).map { |s| [s.id, s] }.flatten] @sources_dirty = false end diff --git a/lib/sup/maildir.rb b/lib/sup/maildir.rb index ba42d79..5bc884b 100644 --- a/lib/sup/maildir.rb +++ b/lib/sup/maildir.rb @@ -64,7 +64,7 @@ class Maildir < Source [ids.sort, ids_to_fns] end rescue SystemCallError => e - die "Problem scanning Maildir directories: #{e.message}." + raise FatalSourceError, "Problem scanning Maildir directories: #{e.message}." end @last_scan = Time.now @@ -72,7 +72,7 @@ class Maildir < Source def each scan_mailbox - start = @ids.index(cur_offset || start_offset) or die "Unknown message id #{cur_offset || start_offset}.", :suggest_rebuild => true # couldn't find the most recent email + start = @ids.index(cur_offset || start_offset) or raise OutOfSyncSourceError, "Unknown message id #{cur_offset || start_offset}." # couldn't find the most recent email start.upto(@ids.length - 1) do |i| id = @ids[i] @@ -95,25 +95,17 @@ class Maildir < Source private - def die message, opts={} - message += " It is likely that messages have been deleted from this Maildir mailbox. Please run sup-sync --changed #{to_s} to correct this problem." if opts[:suggest_rebuild] - self.broken_msg = message - Redwood::log message - BufferManager.flash "Error communicating with Maildir. See log for details." if BufferManager.instantiated? - raise SourceError, message - end - def make_id fn # use 7 digits for the size. why 7? seems nice. sprintf("%d%07d", File.mtime(fn), File.size(fn)).to_i end def with_file_for id - fn = @ids_to_fns[id] or die "No such id: #{id.inspect}.", :suggest_rebuild => true + fn = @ids_to_fns[id] or raise OutOfSyncSourceError, "No such id: #{id.inspect}." begin File.open(fn) { |f| yield f } rescue SystemCallError => e - die "Problem reading file for id #{id.inspect}: #{fn.inspect}: #{e.message}." + raise FatalSourceError, "Problem reading file for id #{id.inspect}: #{fn.inspect}: #{e.message}." end end end diff --git a/lib/sup/mbox/loader.rb b/lib/sup/mbox/loader.rb index 724d49b..dfca5b2 100644 --- a/lib/sup/mbox/loader.rb +++ b/lib/sup/mbox/loader.rb @@ -1,4 +1,5 @@ require 'rmail' +require 'uri' module Redwood module MBox @@ -14,7 +15,7 @@ class Loader < Source when String raise ArgumentError, "not an mbox uri" unless uri_or_fp =~ %r!mbox://! - fn = uri_or_fp.sub(%r!^mbox://!, "") + fn = URI(uri_or_fp).path ## heuristic: use the filename as a label, unless the file ## has a path that probably represents an inbox. @labels << File.basename(fn).intern unless File.dirname(fn) =~ /\b(var|usr|spool)\b/ @@ -22,12 +23,14 @@ class Loader < Source else @f = uri_or_fp end + end + def check if cur_offset > end_offset - self.broken_msg = "mbox file is smaller than last recorded message offset. Messages have probably been deleted via another client. Run 'sup-sync --changed #{to_s}' to correct this." + raise OutOfSyncSourceError, "mbox file is smaller than last recorded message offset. Messages have probably been deleted by another client." end end - + def start_offset; 0; end def end_offset; File.size @f; end @@ -37,9 +40,7 @@ class Loader < Source @f.seek offset l = @f.gets unless l =~ BREAK_RE - Redwood::log "#{to_s}: offset mismatch in mbox file offset #{offset.inspect}: #{l.inspect}" - self.broken_msg = "offset mismatch in mbox file offset #{offset.inspect}: #{l.inspect}. Run 'sup-sync --changed #{to_s}' to correct this." - raise SourceError, self.broken_msg + raise OutOfSyncSourceError, "mismatch in mbox file offset #{offset.inspect}: #{l.inspect}." end header = MBox::read_header @f end @@ -47,7 +48,6 @@ class Loader < Source end def load_message offset - raise SourceError, self.broken_msg if broken? @mutex.synchronize do @f.seek offset begin @@ -55,13 +55,12 @@ class Loader < Source return RMail::Parser.read(input) end rescue RMail::Parser::Error => e - raise SourceError, "error parsing message with rmail: #{e.message}" + raise FatalSourceError, "error parsing mbox file: #{e.message}" end end end def raw_header offset - raise SourceError, self.broken_msg if broken? ret = "" @mutex.synchronize do @f.seek offset @@ -73,7 +72,6 @@ class Loader < Source end def raw_full_message offset - raise SourceError, self.broken_msg if broken? ret = "" @mutex.synchronize do @f.seek offset @@ -86,33 +84,36 @@ class Loader < Source end def next - raise SourceError, self.broken_msg if broken? returned_offset = nil next_offset = cur_offset - @mutex.synchronize do - @f.seek cur_offset - - ## cur_offset could be at one of two places here: - - ## 1. before a \n and a mbox separator, if it was previously at - ## EOF and a new message was added; or, - ## 2. at the beginning of an mbox separator (in all other - ## cases). - - l = @f.gets or raise "next while at EOF" - if l =~ /^\s*$/ # case 1 - returned_offset = @f.tell - @f.gets # now we're at a BREAK_RE, so skip past it - else # case 2 - returned_offset = cur_offset - ## we've already skipped past the BREAK_RE, so just go - end + begin + @mutex.synchronize do + @f.seek cur_offset + + ## cur_offset could be at one of two places here: - while(line = @f.gets) - break if line =~ BREAK_RE - next_offset = @f.tell + ## 1. before a \n and a mbox separator, if it was previously at + ## EOF and a new message was added; or, + ## 2. at the beginning of an mbox separator (in all other + ## cases). + + l = @f.gets or raise "next while at EOF" + if l =~ /^\s*$/ # case 1 + returned_offset = @f.tell + @f.gets # now we're at a BREAK_RE, so skip past it + else # case 2 + returned_offset = cur_offset + ## we've already skipped past the BREAK_RE, so just go + end + + while(line = @f.gets) + break if line =~ BREAK_RE + next_offset = @f.tell + end end + rescue SystemCallError => e + raise FatalSourceError, "Error reading #{@f.path}: #{e.message}" end self.cur_offset = next_offset diff --git a/lib/sup/message.rb b/lib/sup/message.rb index 5a9eda7..decef65 100644 --- a/lib/sup/message.rb +++ b/lib/sup/message.rb @@ -138,7 +138,6 @@ class Message end private :read_header - def broken?; @source.broken?; end def snippet; @snippet || chunks && @snippet; end def is_list_message?; !@list_address.nil?; end def is_draft?; DraftLoader === @source; end @@ -148,7 +147,6 @@ class Message end def save index - return if broken? index.sync_message self if @dirty @dirty = false end @@ -177,8 +175,8 @@ class Message ## this is called when the message body needs to actually be loaded. def load_from_source! @chunks ||= - if @source.broken? - [Text.new(error_message(@source.broken_msg.split("\n")))] + if @source.has_errors? + [Text.new(error_message(@source.error.message.split("\n")))] else begin ## we need to re-read the header because it contains information diff --git a/lib/sup/poll.rb b/lib/sup/poll.rb index 3483383..cae97ae 100644 --- a/lib/sup/poll.rb +++ b/lib/sup/poll.rb @@ -43,7 +43,7 @@ class PollManager @mutex.synchronize do Index.usual_sources.each do |source| # yield "source #{source} is done? #{source.done?} (cur_offset #{source.cur_offset} >= #{source.end_offset})" - yield "Loading from #{source}... " unless source.done? || source.broken? + yield "Loading from #{source}... " unless source.done? || source.has_errors? num = 0 numi = 0 add_messages_from source do |m, offset, entry| @@ -82,11 +82,11 @@ class PollManager ## the index labels, if they exist, so that state is not lost when ## e.g. a new version of a message from a mailing list comes in. def add_messages_from source - return if source.done? || source.broken? + return if source.done? || source.has_errors? begin source.each do |offset, labels| - if source.broken? + if source.has_errors? Redwood::log "error loading messages from #{source}: #{source.broken_msg}" return end diff --git a/lib/sup/source.rb b/lib/sup/source.rb index be01780..dce2f63 100644 --- a/lib/sup/source.rb +++ b/lib/sup/source.rb @@ -1,6 +1,8 @@ module Redwood class SourceError < StandardError; end +class OutOfSyncSourceError < SourceError; end +class FatalSourceError < SourceError; end class Source ## Implementing a new source is typically quite easy, because Sup @@ -30,75 +32,60 @@ class Source ## - load_message offset ## - raw_header offset ## - raw_full_message offset + ## - check ## - next (or each, if you prefer) ## ## ... where "offset" really means unique id. (You can tell I ## started with mbox.) ## - ## You can throw SourceErrors from any of those, but we don't catch - ## anything else, so make sure you catch *all* errors and reraise - ## them as SourceErrors, and set broken_msg to something if the - ## source needs to be rescanned. + ## All exceptions relating to accessing the source must be caught + ## and rethrown as FatalSourceErrors or OutOfSyncSourceErrors. + ## OutOfSyncSourceErrors should be used for problems that a call to + ## sup-sync will fix (namely someone's been playing with the source + ## from another client); FatalSourceErrors can be used for anything + ## else (e.g. the imap server is down or the maildir is missing.) ## - ## Also, be sure to make the source thread-safe, since it WILL be + ## Finally, be sure the source is thread-safe, since it WILL be ## pummeled from multiple threads at once. ## - ## Two examples for you to look at, though sadly neither of them is - ## as simple as I'd like: mbox/loader.rb and imap.rb + ## Examples for you to look at: mbox/loader.rb, imap.rb, and + ## maildir.rb. - - - ## dirty? described whether cur_offset has changed, which means the - ## source info needs to be re-saved to sources.yaml. + ## let's begin! ## - ## broken? means no message can be loaded, e.g. IMAP server is - ## down, mbox file is corrupt and needs to be rescanned, etc. + ## dirty? means cur_offset has changed, so the source info needs to + ## be re-saved to sources.yaml. bool_reader :usual, :archived, :dirty - attr_reader :uri, :cur_offset, :broken_msg + attr_reader :uri, :cur_offset attr_accessor :id def initialize uri, initial_offset=nil, usual=true, archived=false, id=nil @uri = uri - @cur_offset = initial_offset + @cur_offset = initial_offset || start_offset @usual = usual @archived = archived @id = id @dirty = false - @broken_msg = nil end - def broken?; !@broken_msg.nil?; end def to_s; @uri.to_s; end def seek_to! o; self.cur_offset = o; end - def reset! - @broken_msg = nil - begin - seek_to! start_offset - rescue SourceError - end - end + def reset!; seek_to! start_offset; end def == o; o.to_s == to_s; end - def done?; - return true if broken? - begin - (self.cur_offset ||= start_offset) >= end_offset - rescue SourceError => e - true - end - end + def done?; (self.cur_offset ||= start_offset) >= end_offset; end def is_source_for? uri; URI(self.uri) == URI(uri); end + ## check should throw a FatalSourceError or an OutOfSyncSourcError + ## if it can detect a problem. it is called when the sup starts up + ## to proactively notify the user of any source problems. + def check; end + def each - return if broken? - begin - self.cur_offset ||= start_offset - until done? || broken? # just like life! - n, labels = self.next - raise "no message" unless n - yield n, labels - end - rescue SourceError => e - self.broken_msg = e.message + self.cur_offset ||= start_offset + until done? + n, labels = self.next + raise "no message" unless n + yield n, labels end end @@ -108,11 +95,6 @@ protected @cur_offset = o @dirty = true end - - def broken_msg= m - @broken_msg = m -# Redwood::log "#{to_s}: #{m}" - end end Redwood::register_yaml(Source, %w(uri cur_offset usual archived id)) diff --git a/lib/sup/util.rb b/lib/sup/util.rb index c9ac70c..9863282 100644 --- a/lib/sup/util.rb +++ b/lib/sup/util.rb @@ -282,3 +282,31 @@ module Singleton klass.extend ClassMethods end end + +## wraps an object. if it throws an exception, keeps a copy, and +## rethrows it for any further method calls. +class Recoverable + def initialize o + @o = o + @e = nil + end + + def clear_error!; @e = nil; end + def has_errors?; !@e.nil?; end + def error; @e; end + + def method_missing m, *a, &b; __pass m, *a, &b; end + + def id; __pass :id; end + def to_s; __pass :to_s; end + def to_yaml x; __pass :to_yaml, x; end + + def __pass m, *a, &b + begin + @o.send(m, *a, &b) + rescue Exception => e + @e = e + raise e + end + end +end