CFT: fix OfflineIMAP lockup in single-threaded mode

[Eygene Ryabinkin]

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