Subject: Re: [sup-devel] [PATCH] Inotify support for Maildirs. (FIRST DRAFT)

From: Alvaro Herrera <[email-masked]>
Date: Mon, 03 Sep 2012 13:07:28 -0300

Excerpts from Alvaro Herrera's message of lun sep 03 13:02:26 -0300 2012:
> Excerpts from Edward Z. Yang's message of lun sep 03 02:00:59 -0300 2012:
> > The locking is a downright crime (where's the STM when you need it),
> > and it's still racy, but it should work OK.
> 
> Hm.  I tried this but ran into trouble: I currently run branch "next",
> and your patch doesn't apply there; so I tried your ~ezyang fork and
> branch maildir-sync there, but I find that when in that branch (with or
> without this patch), Sup seems to eat 100% of a CPU core doing
> clock_gettime() and select() continuously.  Not sure what's happening.
> I assume you don't see that behavior.

Oh, I see what's going on: it's deleting all my deleted email!  :-)
Nevermind.  I'll just wait for it to finish before trying out your patch.

-- 
Álvaro Herrera <[email-masked]>
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: "Edward Z. Yang" <[email-masked]>
Date: Mon, 3 Sep 2012 00:59:31 -0400

From: "Edward Z. Yang" <[email-masked]>

Signed-off-by: Edward Z. Yang <[email-masked]>
---
 lib/sup/maildir.rb | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 lib/sup/poll.rb    | 33 ++++++++++++++++++++++++++-------
 lib/sup/source.rb  |  4 ++++
 3 files changed, 80 insertions(+), 9 deletions(-)

diff --git a/lib/sup/maildir.rb b/lib/sup/maildir.rb
index 2a91f05..743156d 100644
--- a/lib/sup/maildir.rb
+++ b/lib/sup/maildir.rb
@@ -1,5 +1,6 @@
 require 'uri'
 require 'set'
+require 'inotify'
 
 module Redwood
 
@@ -184,6 +185,45 @@ class Maildir < Source
     nil
   end
 
+  def continuous_poll poll_mutex
+    i = Inotify.new
+    watches = {}
+    @ctimes.each do |d,prev_ctime|
+      subdir = File.join @dir, d
+      wd = i.add_watch(subdir, Inotify::CREATE | Inotify::DELETE | Inotify::MOVE)
+      watches[wd] = d
+    end
+    i.each_event do |ev|
+      poll_mutex.synchronize do
+        @mutex.synchronize do
+          begin
+            ::Thread.current[@dir] = true
+            id = File.join watches[ev.wd], ev.name
+            # check if inotify is stale
+            # since we have @mutex, there is no race (except for
+            # an external program fucking us over)
+            next unless File.exists? File.join(@dir, id)
+            x = Enumerator.new(Index.instance, :each_source_info, self.id, "#{id}").to_a
+            if ev.mask & Inotify::CREATE or ev.mask & Inotify::MOVE_TO
+              next unless x.empty?
+              yield :add,
+                :info => id,
+                :labels => @labels + maildir_labels(id) + [:inbox],
+                :progress => 0
+            elsif ev.mask & Inotify::DELETE or ev.mask & Inotify::MOVE_FROM
+              next unless !x.empty?
+              yield :delete,
+                :info => id,
+                :progress => 0
+            end
+          ensure
+            ::Thread.current[@dir] = nil
+          end
+        end
+      end
+    end
+  end
+
   def labels? id
     maildir_labels id
   end
@@ -248,7 +288,16 @@ private
   end
 
   def maildir_move_file orig_path, new_source_id, flags
-    @mutex.synchronize do
+    if ::Thread.current[@dir]
+      _maildir_move_file orig_path, new_source_id, flags
+    else
+      @mutex.synchronize do
+        _maildir_move_file orig_path, new_source_id, flags
+      end
+    end
+  end
+
+  def _maildir_move_file orig_path, new_source_id, flags
       new_base = (flags.include?("S")) ? "cur" : "new"
       md_base, md_ver, md_flags = maildir_data orig_path
 
@@ -292,7 +341,6 @@ private
       end
 
       [new_source, new_loc]
-    end
   end
 end
 
diff --git a/lib/sup/poll.rb b/lib/sup/poll.rb
index dbd351f..51e0afa 100644
--- a/lib/sup/poll.rb
+++ b/lib/sup/poll.rb
@@ -94,11 +94,27 @@ EOS
         poll if @last_poll.nil? || (Time.now - @last_poll) >= @delay
       end
     end
+    # XXX dup dup
+    SourceManager.usual_sources.each do |source|
+      Redwood::reporting_thread("inotify poll for #{source}") do
+        source.continuous_poll @mutex do |sym, args|
+          poll_handler source, sym, args
+        end
+      end
+    end
+    SourceManager.unusual_sources.each do |source|
+      Redwood::reporting_thread("inotify poll for #{source}") do
+        source.continuous_poll @mutex do |sym, args|
+          poll_handler source, sym, args
+        end
+      end
+    end
   end
 
   def stop
     @thread.kill if @thread
     @thread = nil
+    # handle inotify polls
   end
 
   def do_poll
@@ -172,7 +188,16 @@ EOS
   ## from the index after being yielded.
   def poll_from source, opts={}
     begin
-      source.poll do |sym, args|
+      source.poll do |sym,args|
+        poll_handler source, sym, args
+      end
+      source.go_idle
+    rescue SourceError => e
+      warn "problem getting messages from #{source}: #{e.message}"
+    end
+  end
+
+  def poll_handler source, sym, args
         case sym
         when :add
           m = Message.build_from_source source, args[:info]
@@ -224,12 +249,6 @@ EOS
             UpdateManager.relay self, :updated, m
           end
         end
-      end
-
-      source.go_idle
-    rescue SourceError => e
-      warn "problem getting messages from #{source}: #{e.message}"
-    end
   end
 
   def handle_idle_update sender, idle_since; @should_clear_running_totals = false; end
diff --git a/lib/sup/source.rb b/lib/sup/source.rb
index 06b6e6b..073a10a 100644
--- a/lib/sup/source.rb
+++ b/lib/sup/source.rb
@@ -102,6 +102,10 @@ class Source
     unimplemented
   end
 
+  ## Like poll, but never returns (it is continuous, and uses something
+  ## like inotify. Will always be run in another thread.)
+  def continuous_poll poll_mutex; [] end
+
   def valid? info
     true
   end
-- 
1.7.11.3

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 21 Aug 2012 12:00:31 -0400

Excerpts from Edward Z. Yang's message of mar ago 21 11:17:32 -0400 2012:
> I'm planning on adding inotify support for Maildir mailboxes.
> This would mean we no longer need to 'poll' to find new messages;
> they show up instantly.  Let me know if you're interested and willing
> to help test.

I definitely am interested and willing to test.

---

From: "Edward Z. Yang" <[email-masked]>
Date: Mon, 03 Sep 2012 14:09:57 -0400

OK, cracked a fix; you need this extra patch:

commit f9ea07f3c4982ab46d8171fdba8eabc3af00c840
Author: Edward Z. Yang <[email-masked]>
Date:   Mon Sep 3 14:09:34 2012 -0400

    sync_back after writing to index, not before.

diff --git a/lib/sup/index.rb b/lib/sup/index.rb
index 13798d6..4b533a7 100644
--- a/lib/sup/index.rb
+++ b/lib/sup/index.rb
@@ -657,10 +657,6 @@ EOS
   end
 
   def sync_message m, overwrite
-    ## TODO: we should not save the message if the sync_back failed
-    ## since it would overwrite the location field
-    m.sync_back
-
     doc = synchronize { find_doc(m.id) }
     existed = doc != nil
     doc ||= Xapian::Document.new
@@ -703,6 +699,10 @@ EOS
       @xapian.replace_document docid, doc
     end
 
+    # sync_back must be after label update, so that inotify gets
+    # fresh data from the index
+    m.sync_back
+
     m.labels.each { |l| LabelManager << l }
     true
   end
_______________________________________________
Sup-devel mailing list
[email-masked]
http://rubyforge.org/mailman/listinfo/sup-devel

---

From: ezyang <[email-masked]>
Date: Mon, 03 Sep 2012 12:10:13 -0400

> Nevermind.  I'll just wait for it to finish before trying out your patch.

There is a bug in the patch, where it notices changes that Sup makes from
inotify, and as a result undoes any flag changes you make. So there 
needs to be
a fix here but I'm not sure what it is yet.

Edward

---

From: "Edward Z. Yang" <[email-masked]>
Date: Mon, 03 Sep 2012 14:29:43 -0400

I think I may have seen Sup peg IO R/W in the way you saw, but when I
restarted Sup with debugging, it went away.  Do let me know if you see it
again; I think there's an infinite loop somewhere.

Edward

---

From: "Edward Z. Yang" <[email-masked]>
Date: Tue, 21 Aug 2012 11:17:32 -0400

Cheers,
Edward

---

From: "Edward Z. Yang" <[email-masked]>
Date: Mon, 03 Sep 2012 15:27:13 -0400

Excerpts from Alvaro Herrera's message of Mon Sep 03 14:42:55 -0400 2012:
> For the record, I let it run for a while and after that the poll log had
> a couple thousand lines saying "Deleting <some-msg-id> <its subject>".
> After that, so far it behaves normally.  I don't know what prompted
> those particular messages to be deleted; I certainly have lots of
> messages "is:deleted" yet.

Confirmed it's Xapian taking it's sweet time to delete the mail.
You might do better with a higher XAPIAN_FLUSH_THRESHOLD value in bin/sup,
but the process seems fairly CPU bound.

Edward

---

From: "Edward Z. Yang" <[email-masked]>
Date: Mon, 03 Sep 2012 19:31:28 -0400

Extra note: the ruby-inotify plugin appears to be braindead
on at least Ruby 1.8.7 for Ubuntu Precise, and will spin-loop.  If
sup is chomping 15% CPU on idle, try this patch on ruby-inotify

Reported here: https://github.com/ruby-building-blocks/ruby-inotify/issues/8

diff --git a/lib/inotify/inotify_native.rb b/lib/inotify/inotify_native.rb
index 66db2b4..27d1043 100644
--- a/lib/inotify/inotify_native.rb
+++ b/lib/inotify/inotify_native.rb
@@ -115,7 +115,7 @@ require 'ffi'
     # each_event() provides an easy way to loop over all events as they occur
     def each_event
       loop do
-        ready = IO.select([@io], nil, nil, nil)
+        ready = IO.select([@io])
         event = self.read_event
         yield event
       end

---

From: "Edward Z. Yang" <[email-masked]>
Date: Mon, 03 Sep 2012 14:49:42 -0400

Excerpts from Alvaro Herrera's message of Mon Sep 03 14:42:55 -0400 2012:
> Message at 0 has changed its source location. Updating labels from draft,personal => draft,personal
> Message at 1 has changed its source location. Updating labels from draft,inbox,personal => draft,inbox,personal
> Message at 2 has changed its source location. Updating labels from draft,personal => draft,personal
> Message at 3 has changed its source location. Updating labels from draft,personal => draft,personal

That's odd, I guess Enumerator.new(Index.instance, :each_source_info, self.id).to_a
returning a null set so we always emit :add?  If you could add a debug call on
that enumerator and get me the contents of old_ids and new_ids whereabouts draft.rb
line 70 that would be great.

Edward

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 04 Sep 2012 15:09:54 -0300

Excerpts from Edward Z. Yang's message of mar sep 04 13:06:07 -0300 2012:
> I've pushed a new revision of the branch: http://gitorious.org/~ezyang/sup/ezyang/commits/maildir-sync
> which solves some bad interactions with sync-back.

Okay, I'm running this now.  Having the emails show up instantly works
great ... hooray for continuous, permanent distraction ;-)

I haven't debugged the other issue yet.  I'll let you know.

---

From: "Edward Z. Yang" <[email-masked]>
Date: Mon, 03 Sep 2012 01:00:59 -0400

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 03 Sep 2012 15:42:55 -0300

> again; I think there's an infinite loop somewhere.

Sure.

Another curious thing with your branch is that the poll log gets these
four lines each time a poll takes place:

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 03 Sep 2012 13:02:26 -0300

---

From: "Edward Z. Yang" <[email-masked]>
Date: Tue, 04 Sep 2012 12:06:07 -0400

Edward