CFT: fix OfflineIMAP lockup in single-threaded mode

[Eygene Ryabinkin]

Tue, Jan 29, 2013 at 11:39:14AM +0100, X Ryl wrote:
> But at the same time, the number of people changing code will be
> small, so it's likely we'll spot a change in the suggestthreads of
> any folder variant.

May be, or may be not.  Incidentially, I had forgot to add the whole
offlineimap/utils folder to the commit diff that introduces depots,
  http://codelabs.ru/patches/offlineimap/2013-use-singleton-depot-for-passing-options.diff
(had already updated it with all needed files) there are two people
who are reviewing this code and approach, but seems like no one had
really checked the whole patch or tried to apply it and have a go.

> I'm inclined to prefer the simple "if" variant anyway, since
> spreading config testing everwhere is, IMHO a bad design anyway and
> hard to maintain.

Could you, please, elaborate on the "bad design" and "hard to maintain"?

> So the improved safety here might not worth it.

For me, any improved safety and general uniformity of code (remember,
we have two places where we do single/multithreaded switch,
folder/Base.py and accounts.py) outweights the rest in all, but rare
cases of performance-critical path.  So relying only at suggestthreads()
is a premature optimization as Dr. Knuth 'sez ;))

> > 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().
> 
> Not sure about this. If you're asking the software to use a single thread
> mode, then the class should never suggest threads.

It is the matter of semantics of suggestthreads().  For me, it is just
a suggestion from the folder engine that says "I will/won't benefit
from using multithreaded code", that's it.  We can try to modify the
code not to suggest to use multithreading when we know that OI is
operating in single-threaded mode, but it is just a safety belt for
people who aren't share the abovementioned point of view on 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.
>
> However, I still don't like config testing everywhere in the code as
> it's a pain to maintain and document.

Again, the explicit arguments for "pain to maintain and document" for
this very case will be appreciated.

> Let's see below how to solve this.

The below suggestions are talking about depot/singleton/etc, but
aren't touching the first question on if we want to keep the test for
options.singlethreading in folder/Base.py.  And these problems are
orthogonal to each other: first addresses the "when we should check
options.singlethreading", the second asks about "how we will pass
that bit of information".

> > 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.
> >
> 
> Well, I disagree. You are making a second instance which refers to
> the same object.

No.  When you have two objects instances that are equal to each other
(in all senses, depot.Options() == depot.Options() must be true
all the time and the internal state must be precisely the same and
should refer to the same objects), then it is the same instance.
Just because you can't distinguish them from each other.

> The implementation details are details, which will never be read by
> a casual python developer (who's is going to check a file called
> __init.py to figure out the "trick" of the singleton instance?).

They, generally, shouldn't, if there will be documentation piece that
says: use offlineimap.utils.depot.Options() to get the object that has
the get(option_name) method that will return to you the value of the
requested option.  That _is_ the goodness of encapsulation and
guaranteed public API.

> It's currently confusing when you read the code that's using the
> depot.Option() instance.  Confusing about the case, and confusing
> about the logic (if I don't know how the Option() is implemented,
> which is not usual) of the object usage.

As you understand, I am not confused about Options(), because I wrote
the code ;))  Thus, I am very much interested what confusion it leads
to, given that there should be the public API description for
depot.Options (that isn't here yet)?

> 1) Split config.get/set interface, in more rigid, global, config.general,
> config.repositories, config.accounts objects
>    Pro: Documenting the config option is easier by simply grepping the
> code, instead of having to run through a debugger to spot each get() usage.
>    Con: Rewrite of the config usage in the code
> 2) Identify which part should be const, and which should not (I think
> everything is supposed to be const, but I'm probably missing something)
> 3) Constify the config part that are supposed to be const (again, I don't
> get the need of a singleton here, compared to a global variable)
>    Pro: Will raise const error if any code is doing config mixup, so we can
> check any "unusual" pattern / bug
>    Con: I don't see any. A developer does not need to know the
> implementation details of a const object (it appears as a usual object),
> and an exception is easy to track and debug.
> 4) Add a config.runtime const object that's made for runtime passed
> arguments (like singlethread, singleaccount, etc..) and let the code refer
> to this.
>    Pro: It's cleaner than the config "general" section hack I've made for
> singlethread mode, and the code will be easier to maintain.
>    Con: Need some code refactoring, but not much

I think that I'll try to provide the read-only access to the 'options'
via some global variable, say offlineimap.globals.options, and will
const'ify the whole contents.  Everyone who will need write access
(and there should be almost no such methods across the whole OI code)
will either pass 'options' explicitel before they are frozen or come
to the other way to do it, if it will be needed.

Thanks.
-- 
rea