CFT: fix OfflineIMAP lockup in single-threaded mode

[Eygene Ryabinkin]

Cyril, good day.

Mon, Jan 28, 2013 at 09:37:18PM +0100, X Ryl wrote:
> I'm ok with the first patch, but I would merge it with the last one, and
> update the part that read:
> 
>  if self.suggeststhreads() and self.config.get('general',
> 'single-thread') == 'False':
> 
> to simply this:
> 
>  if self.suggeststhreads():
> 
> suggeststhreads() should do the self.config.get uglyness in this patch, so
> it's self sufficient (one can apply it and does not depend on any later
> patch).

In my view, such a change slightly mixes two things:
 - being in single-threaded mode by virtue of config.singlethreading;
 - taking a hint from the folder if it will have any benefit from
   multithreaded processing.

Sure, my last patch
  http://codelabs.ru/patches/offlineimap/2013-IMAP-dont-suggest-multithreading-in-singlethreaded-mode.diff
currently makes the second hint to be consistent with the
config.singlethreading.  But since some other, future folder variant
can (errorneously) suggest mutithreaded processing every time, the
shorter 'if' variant will be broken again.

We could try to solve this problem by forcing suggestthreads() to return
zero when config.singlethreading is True, but for me it is a slight
deviation from the semantics of suggestthreads().

I had been thinking of the shorter 'if' variant when I was extending
your patch, but came to the conclusion that the explicit check for being
single-threaded will be better: it has the proper semantics, it is more
explicit and it coincides with the way the account.py was modified, so
this provides some uniformity of code.

That was my reasoning to be explicit.


> Then, the depot/Option part, if you don't mind, need a bit a work, before
> being applied:
> If I understand what you're doing, your Singleton class has a static dict
> member called "__options",

No.  __options is not a static member, two underscores mean "this variable
is private by-convention and name manger will be active on it":
  http://docs.python.org/2/tutorial/classes.html#private-variables-and-class-local-references

> and each time you need to check for an "option", you create an
> instance of the Option class that simply refer to this static
> dictionnary.

No, there is only one instance of any singleton -- that's the "meat"
of this design pattern.  At any time, depot.Options() ==
depot.Options, and in our pythonic case the magic is in the __new__
method that just saves the first instance and returns it every time
when a new instantiation is tried to be made.

> This is, a bit unusual. If one changes the Option() instance, it's only
> local, as a static member change make it instance member, right ?

One really can't change the Option() instance in any way by using its
public API: there is no write accessor for the stored options, only
getter.  Of course, it still could be changed, but it will involve some
extra code, so people who will want to abuse this, should have to think
and people who will review the code will see that something unusual is
happening, so such part should be carefully reviewed.  At least, I hope
for that.

> In that case, it's not const (read only). In the same module, a change of
> the option variable would still work, so it won't break as soon as you're
> changing it (but at least it'll not break the other modules) .

I am not trying to fight the changes of self.__options in the same class:
such code is local and, probably, programmer will know what is going on.

I am trying to protect the stored options from unintentional changes
from the outside of the depot to avoid people to modify options from
the code that isn't in offlineimap/init.py by accident or without
careful evaluation of consequences.  May be this is an overkill.

> Let me know if I understand correctly, so we can have a better
> implementation of the "depot" idea.

I think that after reading the above, you will get better
understanding of my intentions.

> I find the idea of a global "depot" more interesting than
> instantiating a new object each time needed.

There is only single instantiation, as I explained.  And I was trying
to do not only the single "depot", but a foundation for multiple,
logically not-very-much-related depots if they will be needed in
future by adding a thin layer now.  Perhaps I should find my instance
of Occam's razor and try to apply it here ;))

Thanks!
-- 
rea