CFT: fix OfflineIMAP lockup in single-threaded mode

[X Ryl]

On Wed, Jan 30, 2013 at 8:42 AM, Eygene Ryabinkin <rea at codelabs.ru> wrote:

> > 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"?
>
> When you have such code (which is mine, and I'm not proud):

key = "singlethread"  # Or worse, can come from the top class, whatever
config.get("Repository " + repository.getname(), key)

Let's say later on, we decide to rename the config option to "nothread"
Then if you grep the code, it's impossible to find such a pattern, so
you'll likely introduce a bug.

Compared to:
config.repository[repository.getname()].singlethread

Then, you can grep the code and you'll find all the places where such "key"
is used.
Better, when you need to document all the config option (a work I'm
currently trying to do), you can grep for "config\." and get a list of all
the config options directly, you don't have to run the software with
breakpoint to figure out what was the value of 'key' at runtime.

I consider the later code easier to maintain, obviously, and the design,
being a bit more rigid, would impose a better code, since you must know at
write time what is the key you're reading, and if you do something wrong,
it raises immediately (in the former case, you'd get empty value, which
will likely break, *way later*).




> > 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 ;))
>
> Agreed.



> > 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.
>
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.
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()
  imapFolder.syncmessage()

To that:
In offlineimap.py.run:
  if config.get("general", "single-thread") == "True":
      # run single threaded
  else:
      # start thread pool

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)
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)
3) You don't need to pass your config object around (again, easier to
maintain if you need to change the config object)

What do you think?

Cyril
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://alioth-lists.debian.net/pipermail/offlineimap-project/attachments/20130130/a382f0b6/attachment-0003.html>