[PATCH 0/13] Re: Reintegrate imaplib2 and IDLE, again
nicolas.s-dev at laposte.net
Sun Feb 6 21:15:00 GMT 2011
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
> 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
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
> - 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
> 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.
More information about the OfflineIMAP-project