[sup-devel] [PATCH] don't leak fds for mbox sources

* [sup-devel] [PATCH] don't leak fds for mbox sources
@ 2010-07-08 17:15 Sascha Silbe
  2010-07-13  2:23 ` Rich Lane
  0 siblings, 1 reply; 5+ messages in thread
From: Sascha Silbe @ 2010-07-08 17:15 UTC (permalink / raw)
  To: sup-devel

Only open the mbox when it's actually used and close it again once we're
finished iterating over it.

Adds a method go_idle to Source to inform the source we're unlikely to
use it in the near future so it can do internal clean-up as appropriate.

Signed-off-by: Sascha Silbe <sascha-pgp@silbe.org>
---
 lib/sup/mbox.rb   |   28 ++++++++++++++++++++++++++--
 lib/sup/poll.rb   |    7 ++++++-
 lib/sup/source.rb |    6 ++++++
 3 files changed, 38 insertions(+), 3 deletions(-)

This patch allows configuring more than RLIMIT_NOFILE mbox sources. I have
been testing it for several days.

Please do point out even minor style mistakes.

diff --git a/lib/sup/mbox.rb b/lib/sup/mbox.rb
index 3f7ae3e..2bf32ea 100644
--- a/lib/sup/mbox.rb
+++ b/lib/sup/mbox.rb
@@ -23,7 +23,7 @@ class MBox < Source
       raise ArgumentError, "not an mbox uri" unless uri.scheme == "mbox"
       raise ArgumentError, "mbox URI ('#{uri}') cannot have a host: #{uri.host}" if uri.host
       raise ArgumentError, "mbox URI must have a path component" unless uri.path
-      @f = File.open uri.path, 'rb'
+      @f = nil
       @path = uri.path
     else
       @f = uri_or_fp
@@ -53,12 +53,31 @@ class MBox < Source
     end
   end
 
+  def ensure_open
+    @f = File.open @path, 'rb' if @f.nil?
+  end
+  private :ensure_open
+
+  def go_idle
+    @mutex.synchronize do
+      return if @f.nil? || @path.nil?
+      @f.close
+      @f = nil
+    end
+  end
+
   def start_offset; 0; end
-  def end_offset; File.size @f; end
+  def end_offset
+    @mutex.synchronize do
+      ensure_open
+      File.size @f
+    end
+  end
 
   def load_header offset
     header = nil
     @mutex.synchronize do
+      ensure_open
       @f.seek offset
       l = @f.gets
       unless MBox::is_break_line? l
@@ -71,6 +90,7 @@ class MBox < Source
 
   def load_message offset
     @mutex.synchronize do
+      ensure_open
       @f.seek offset
       begin
         ## don't use RMail::Mailbox::MBoxReader because it doesn't properly ignore
@@ -88,6 +108,7 @@ class MBox < Source
   ## scan forward until we're at the valid start of a message
   def correct_offset!
     @mutex.synchronize do
+      ensure_open
       @f.seek cur_offset
       string = ""
       until @f.eof? || MBox::is_break_line?(l = @f.gets)
@@ -100,6 +121,7 @@ class MBox < Source
   def raw_header offset
     ret = ""
     @mutex.synchronize do
+      ensure_open
       @f.seek offset
       until @f.eof? || (l = @f.gets) =~ /^\r*$/
         ret << l
@@ -131,6 +153,7 @@ class MBox < Source
   ## sup-sync-back has to do it.
   def each_raw_message_line offset
     @mutex.synchronize do
+      ensure_open
       @f.seek offset
       yield @f.gets
       until @f.eof? || MBox::is_break_line?(l = @f.gets)
@@ -145,6 +168,7 @@ class MBox < Source
 
     begin
       @mutex.synchronize do
+        ensure_open
         @f.seek cur_offset
 
         ## cur_offset could be at one of two places here:
diff --git a/lib/sup/poll.rb b/lib/sup/poll.rb
index 49e456b..3ce867c 100644
--- a/lib/sup/poll.rb
+++ b/lib/sup/poll.rb
@@ -159,7 +159,10 @@ EOS
   ## this is the primary mechanism for iterating over messages from a source.
   def each_message_from source, opts={}
     begin
-      return if source.done? || source.has_errors?
+      if source.done? || source.has_errors?
+        source.go_idle
+        return
+      end
 
       source.each do |offset, source_labels|
         if source.has_errors?
@@ -175,6 +178,8 @@ EOS
         HookManager.run "before-add-message", :message => m
         yield m
       end
+
+      source.go_idle
     rescue SourceError => e
       warn "problem getting messages from #{source}: #{e.message}"
       Redwood::report_broken_sources :force_to_top => true
diff --git a/lib/sup/source.rb b/lib/sup/source.rb
index 2f2e5df..13dc443 100644
--- a/lib/sup/source.rb
+++ b/lib/sup/source.rb
@@ -40,6 +40,7 @@ class Source
   ## - raw_header offset
   ## - raw_message offset
   ## - check (optional)
+  ## - go_idle (optional)
   ## - next (or each, if you prefer): should return a message and an
   ##   array of labels.
   ##
@@ -94,6 +95,11 @@ class Source
   ## to proactively notify the user of any source problems.
   def check; end
 
+  ## release resources that are easy to reacquire. it is called
+  ## after processing a source (e.g. polling) to prevent resource
+  ## leaks (esp. file descriptors).
+  def go_idle; end
+
   ## yields successive offsets and labels, starting at #cur_offset.
   ##
   ## when implementing a source, you can overwrite either #each or #next. the
-- 
1.7.1

_______________________________________________
Sup-devel mailing list
Sup-devel@rubyforge.org
http://rubyforge.org/mailman/listinfo/sup-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread