CFT: fix OfflineIMAP lockup in single-threaded mode

[X Ryl]

Hi,

 I agree with all the point you raised.
The patch you've written is clear to me, and I think it's the best solution
(concerning singlethread + suggestthread).
Fill free to commit it (unless anyone objects).
In a later patch we could probably remove suggestthread (if we have 2
class, single & multi), so your current patch would still be correct.

Cheers,
Cyril

On Tue, Feb 5, 2013 at 8:47 PM, Eygene Ryabinkin <rea at codelabs.ru> wrote:

> Wed, Jan 30, 2013 at 10:20:06AM +0100, X Ryl wrote:
> > On Wed, Jan 30, 2013 at 8:42 AM, Eygene Ryabinkin <rea at codelabs.ru>
> wrote:
> > > Again, the explicit arguments for "pain to maintain and document" for
> > > this very case will be appreciated.
> > >
> > The more test you have spread in the code, based on an config option, the
> > more likely you'll miss one if you change the config.
>
> If the configuration keys are grep'able, the likelihood of miss is somewhat
> diminishes.
>
> > Compare this:
> >
> > class SingleThreadImap:
> >    # do single thread operations
> >
> > class MultiThreadImap: # inherit SingleThreadImap
> >   # override the methods that can benefit from multithreading
> >
> > In syncaccount function:
> >   imapFolder = SingleThreadImap() if config.runtime.singlethread else
> > MultiThreadImap()
>
> Such technique puts too much knowledge about the internal implementation
> of *ThreadImap() class to the syncaccount.  The better way is to pass
> the 'config' to the IMAP constructor and it will decide what object
> to instantiate basing on the passed arguments.
>
> >   imapFolder.syncmessage()
> >
> > To that:
> > In offlineimap.py.run:
> >   if config.get("general", "single-thread") == "True":
> >       # run single threaded
> >   else:
> >       # start thread pool
>
> There is no direct equivalent to this block in the previous code snippet,
> but you'll still need this (though not in offlineimap.py.run(), but
> in offlineimap/accounts.py.
>
> > In the syncaccount function
> >   if imapFolder.suggestthreads() and not config.get("general",
> > "singlethread") == "True":
> >      # launch thread for syncmessage
> >   else:
> >      # launch syncmessage here
> >
> > I think the former is easier:
> > 1)  it's more clear (you have a file called "SingleThreadImap" and
> another
> > one called "MultiThreadImap", which is obvious when you're looking for a
> > bug in either mode)
>
> Yes.  If we can manage to split the class into two pieces that aren't
> duplicating each other.  Seems like we will be able, but I hadn't
> tried to really do that.
>
> > 2) you only test the config once (re-read the second code above, and
> you'll
> > see that I've introduced a bug that would be quite impossible to debug
> if I
> > haven't told you)
>
> You'll face an exception if you really have no "single-thread" variable
> inside the "general" section.
>
> > 3) You don't need to pass your config object around (again, easier to
> > maintain if you need to change the config object)
>
> We're already passing config object to all places that are relevant to the
> current bug.  But, if we want to check 'options' (that are the primary
> source
> of 'singlethreading'), you can use another patch,
>
> http://codelabs.ru/patches/offlineimap/2013-altpatch-create-global-options-instance.diff
> that I had just posted in the other message in this thread.
>
> > What do you think?
>
> I think that we should fix the lockup in the single-threaded mode in
> one commit that is "minimal" in some sense.  And we already seem to
> agree on all points, but the one: do we want to test both
> 'singlethreading' and suggestthreads() or just suggestthreads().
>
> And after this we can think about splitting IMAP code into pieces,
> refactoring the other parts, etc.  But in new commits, let's not mix
> the things.
>
> Let's finish with the problem in $subject first.
> --
> rea
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://alioth-lists.debian.net/pipermail/offlineimap-project/attachments/20130206/86e9e188/attachment-0003.html>