CFT: fix OfflineIMAP lockup in single-threaded mode

[Eygene Ryabinkin]

Nicolas, good day.

Sun, Jan 27, 2013 at 01:09:22PM +0100, Nicolas Sebrecht wrote:
> On Sun, Jan 27, 2013 at 01:08:12AM +0400, Eygene Ryabinkin wrote:
> > I was trying to integrate the fix for the issue #22,
> >   https://github.com/OfflineIMAP/offlineimap/issues/22
> > but came up with the updated patch,
> >   http://codelabs.ru/patches/offlineimap/2012-preliminary-fix-deadlock-singlethreaded-IMAP-sync.diff
> 
> Very interesting work! Improving the threads system is something I've
> tried to do in the past whithout beeing able to come with relevant
> patches.

It is based on Cyril's work and analysis, so the real credit in this
case should go to him.

> > In a nutshell, if you're syncing more than 100 (accounts + messages),
> > you will experience a lockup if offlineimap is executed with flag
> > "-1".
> 
> It is still not clear to me where this _100_ limit comes from.

From the
{{{
exitthreads = Queue(100)
}}}
inside offlineimap/threadutil.py.  The thing is that each
ExitNotifyThread pushes itself to this queue upon exit to save exit
notification.  And without exitnotifymonitorloop() that consumes these
notification the queue will be full after 100 clients.  And since each
InstanceLimitedThread is the child of the ExitNotifyThread and it
acquires the BoundedSemaphore on its start(), there will be a
situation when such thread will be trying to put itself to the
'exitthreads' and the queue will be full and that thread will exhaust
the BoundedSemaphore, so no new threads will be spawned before it will
exit, but it can't, since it waits for the next free slot in the Queue
that isn't cleaned.

> >        This patch tries to catch all such problems.  It is not yet in
> > the 'next' branch, since I'll be polishing it a bit, but the wider
> > testing for it is welcome, both in single-threaded (via "-1") and your
> > regular mode of operations.  Comments and reviews are welcome too.
> 
> From my very fast review I'd say the patch looks good enough to be
> merged.

One thing that worries me currently is the way that options.singlethreading
is propagated from the main code: adding new pseudo-item to the 'general'
configuration section isn't that good.  Something like the singleton that
will provide read-only access to the options is a better way, in my view.

> If you intend to get it tested, I would suggest you to merge it to the
> next branch. It is the purpose of this branch: get things tested before
> integration into master.

I will after fixing the above-mentioned thing.

> Also, the release cycle is still at release candidate v2 and with the
> lack of activity these last months (from the repository POV), I think we
> can consider this -rc2 to be a very early release candidate (so, open to
> crazy/broken patches).

Fine.  I had started to get through all open bugs at GitHub and trying to
fix them.

> If it appears the patch is not correct, you could 'git revert' the
> commit.
> 
> From my experience, I suggest this approach because OfflineIMAP has a
> small amount of active contributors. If you don't do the job for the
> others, few will actually read the patch and fewer will apply it and
> test it. Having it merged gives much more chances for the patch to be
> tested.

OK, the point is taken.

Thanks!
-- 
rea