CFT: fix OfflineIMAP lockup in single-threaded mode
boite.pour.spam at gmail.com
Wed Feb 6 09:24:05 GMT 2013
I agree with all the point you raised.
The patch you've written is clear to me, and I think it's the best solution
(concerning singlethread + suggestthread).
Fill free to commit it (unless anyone objects).
In a later patch we could probably remove suggestthread (if we have 2
class, single & multi), so your current patch would still be correct.
On Tue, Feb 5, 2013 at 8:47 PM, Eygene Ryabinkin <rea at codelabs.ru> wrote:
> 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>
> > > 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
> > 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
> > 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
> > 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
> of 'singlethreading'), you can use another patch,
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the OfflineIMAP-project