Hi,<div><br></div><div> I agree with all the point you raised.</div><div>The patch you've written is clear to me, and I think it's the best solution (concerning singlethread + suggestthread).</div><div>Fill free to commit it (unless anyone objects).</div>
<div>In a later patch we could probably remove suggestthread (if we have 2 class, single & multi), so your current patch would still be correct. </div><div><br></div><div>Cheers,</div><div>Cyril<br><br><div class="gmail_quote">
On Tue, Feb 5, 2013 at 8:47 PM, Eygene Ryabinkin <span dir="ltr"><<a href="mailto:rea@codelabs.ru" target="_blank">rea@codelabs.ru</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">Wed, Jan 30, 2013 at 10:20:06AM +0100, X Ryl wrote:<br>
</div><div class="im">> On Wed, Jan 30, 2013 at 8:42 AM, Eygene Ryabinkin <<a href="mailto:rea@codelabs.ru">rea@codelabs.ru</a>> wrote:<br>
</div><div class="im">> > Again, the explicit arguments for "pain to maintain and document" for<br>
> > this very case will be appreciated.<br>
> ><br>
> The more test you have spread in the code, based on an config option, the<br>
> more likely you'll miss one if you change the config.<br>
<br>
</div>If the configuration keys are grep'able, the likelihood of miss is somewhat<br>
diminishes.<br>
<div class="im"><br>
> Compare this:<br>
><br>
> class SingleThreadImap:<br>
> # do single thread operations<br>
><br>
> class MultiThreadImap: # inherit SingleThreadImap<br>
> # override the methods that can benefit from multithreading<br>
><br>
> In syncaccount function:<br>
> imapFolder = SingleThreadImap() if config.runtime.singlethread else<br>
> MultiThreadImap()<br>
<br>
</div>Such technique puts too much knowledge about the internal implementation<br>
of *ThreadImap() class to the syncaccount. The better way is to pass<br>
the 'config' to the IMAP constructor and it will decide what object<br>
to instantiate basing on the passed arguments.<br>
<div class="im"><br>
> imapFolder.syncmessage()<br>
><br>
> To that:<br>
> In offlineimap.py.run:<br>
> if config.get("general", "single-thread") == "True":<br>
> # run single threaded<br>
> else:<br>
> # start thread pool<br>
<br>
</div>There is no direct equivalent to this block in the previous code snippet,<br>
but you'll still need this (though not in offlineimap.py.run(), but<br>
in offlineimap/accounts.py.<br>
<div class="im"><br>
> In the syncaccount function<br>
> if imapFolder.suggestthreads() and not config.get("general",<br>
> "singlethread") == "True":<br>
> # launch thread for syncmessage<br>
> else:<br>
> # launch syncmessage here<br>
><br>
> I think the former is easier:<br>
> 1) it's more clear (you have a file called "SingleThreadImap" and another<br>
> one called "MultiThreadImap", which is obvious when you're looking for a<br>
> bug in either mode)<br>
<br>
</div>Yes. If we can manage to split the class into two pieces that aren't<br>
duplicating each other. Seems like we will be able, but I hadn't<br>
tried to really do that.<br>
<div class="im"><br>
> 2) you only test the config once (re-read the second code above, and you'll<br>
> see that I've introduced a bug that would be quite impossible to debug if I<br>
> haven't told you)<br>
<br>
</div>You'll face an exception if you really have no "single-thread" variable<br>
inside the "general" section.<br>
<div class="im"><br>
> 3) You don't need to pass your config object around (again, easier to<br>
> maintain if you need to change the config object)<br>
<br>
</div>We're already passing config object to all places that are relevant to the<br>
current bug. But, if we want to check 'options' (that are the primary source<br>
of 'singlethreading'), you can use another patch,<br>
<a href="http://codelabs.ru/patches/offlineimap/2013-altpatch-create-global-options-instance.diff" target="_blank">http://codelabs.ru/patches/offlineimap/2013-altpatch-create-global-options-instance.diff</a><br>
that I had just posted in the other message in this thread.<br>
<br>
> What do you think?<br>
<br>
I think that we should fix the lockup in the single-threaded mode in<br>
one commit that is "minimal" in some sense. And we already seem to<br>
agree on all points, but the one: do we want to test both<br>
'singlethreading' and suggestthreads() or just suggestthreads().<br>
<br>
And after this we can think about splitting IMAP code into pieces,<br>
refactoring the other parts, etc. But in new commits, let's not mix<br>
the things.<br>
<br>
Let's finish with the problem in $subject first.<br>
<span class="HOEnZb"><font color="#888888">--<br>
rea<br>
</font></span></blockquote></div><br></div>