[PATCH 0/13] Re: Reintegrate imaplib2 and IDLE, again

[Nicolas Sebrecht]

On Sun, Feb 06, 2011 at 11:58:54AM -0500, Ethan Glasser-Camp wrote:

> Attached please find my newest crack at the integration of IDLE into
> offlineimap. Sorry for the months of delay. This is based on the
> original patch series by James Bunton <jamesbunton at fastmail.fm>, but
> rebased heavily, with the following goals:
> 
> - Add imaplib2 only one time, as its own commit.
> 
> - Smaller, more atomic patches, instead of keeping the original
>   commits "as is".
> 
> - Based on current master.

Thanks; it looks much better at a first quick review but most patches
miss the Signed-off-by line unless you don't intend the topic to be
merged.

> This patch series adds support for IMAP IDLE for accounts that are
> IMAP<->Maildir. (IMAP<->IMAP has some complications that I am working
> on with Tom Lawton.) It is turned on by the "idlefolders"
> configuration option for a given repository. As previously discussed,
> this branch relies on imaplib2 replacing imaplib. This has the
> potential to break everything for everyone, but it seems to work OK
> for me right now.
> 
> The approach by this patch series is:
> 
> - Patch 2: Introduce imaplib2.
> 
> - Patch 3: Import imaplib2 instead of imaplib. imaplib2 has slightly
>   different semantics than standard imaplib, so this patch will break
>   the build, but I thought it was helpful to have it as a separate
>   commit.

Should be told in the commit message.

> - Patch 4: Match semantics of imaplib2. This is the most complicated
>   patch, since it changes three or four distinct things, but since it
>   fixes the build, I didn't think it made sense to break it up.
> 
> - Patch 5: Remove WrappedIMAP4_SSL.read(). This overridden method
>   seemed to be built to interact with imaplib, but with imaplib2,
>   it prevents SSL from working. We seem to be OK without it...
> 
> - Patch 6: Read and store configuration option "idlefolders", but
>   don't use it yet.
> 
> - Patch 7: Introduce the IdleThread class, which will handle acquiring
>   a connection, spawning a thread, etc. instead of this functionality
>   being in IMAPServer.keepalive(). Additionally, it can respond to
>   IDLE notifications.
> 
> - Patch 8: Replace the thread-spawning code in IMAPServer.keepalive()
>   to use IdleThreads instead, for any folders listed in
>   "idlefolders". This is the patch that actually enables IDLE.
> 
> - Patch 9-13: Various minor enhancements. Among others, handle error
>   cases like connection timeouts and an "abort" message we sometimes
>   get from GMail.
> 
> The last patch is by Tom Lawton <tlawton at gmx.de>. My email setup here
> is a little rough, so I'm sorry if that information gets lost.

The information is OK but Tom's Signed-off-by is missing (added him in
cc).

> Caveats:
> 
> - imaplib2 spawns three threads per connection. Threads are also
>   spawned by the keepalive() method in imapserver, but this patch
>   series doesn't make that worse. This might be seen as a regression
>   for the single-threaded option :)

Library threads aren't ours. As long as we don't break the
single-threaded option for our code, I guess it's OK. Actually, I didn't
check if what I'm saying makes sense in this case, though. :)

So, do we always _need_ threads enabled in our code in order to have
IDLE?  Can IDLE honor single-threaded option?

> - This may still cause certain weird hangs. Before I started rebasing
>   this, I had an occasional deadlock that I traced to the
>   semaphore/connectionlock. I hope it may be fixed by the last patch,
>   but I haven't tested thoroughly enough to be sure.
> 
> - The original patch series by James Bunton removed what are now
>   WrappedIMAP4.open() and WrappedIMAP4_SSL.open(), which override the
>   methods in imaplib2. We use these methods to provide IPv6 support
>   and to verify certs (in the case of SSL). I'm not sure how much
>   overlap there is between these methods and those provided by
>   imaplib2. I decided to leave them in because I didn't want to make
>   waves and because I don't have an IPv6 setup handy to test with. So
>   there's a refactoring that could be there.
> 
> - There are still three other changes in the original patches that
>   look like they could be refactorings, but don't have any obvious
>   effects or explanations. I left them out of this patch series and
>   nothing seems to be broken. These patches are available as the
>   readd-leftovers branch on my Github fork.
> 
> - I think IMAP-over-tunnel might be broken, and that removing
>   IMAP4_Tunnel.read would fix it. I can't test this.

Sorry for the dump question: what is IMAP-over-tunnel in the first
place?

> I'd love to have more testing from people who use IPv6 or IMAP_Tunnel,
> since this branch has the potential to break everything for them, even
> if they don't enable IDLE. Other than that, I'd like to propose that
> this series be merged into pu :)

I'll do merge as soon as I'll have the missing Signed-off-by(s). In
order to have it more widely tested, we could merge it in the mainline
_IF_ IDLE option is marked as EXPERIMENTAL stuff (after the coming
stable release, of course).

Thanks both for the good work.

-- 
Nicolas Sebrecht