CFT: fix OfflineIMAP lockup in single-threaded mode

[X Ryl]

On Tue, Jan 29, 2013 at 6:44 AM, Eygene Ryabinkin <rea at codelabs.ru> wrote:

> 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.
>
Ok, point taken.
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.
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.
So the improved safety here might not worth it.

>
> 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.


> 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.
>
> Again, I agree with your point.
However, I still don't like config testing everywhere in the code as it's a
pain to maintain and document.
Let's see below how to solve this.


> 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.
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?).

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.



> 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.
>
> Ok, what do you think of the later:
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

Cheers,
Cyril
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://alioth-lists.debian.net/pipermail/offlineimap-project/attachments/20130129/1053aedb/attachment-0003.html>