Re: [Evolution-hackers] imapx: Avoid running FETCH_NEW_MESSAGES and REFRESH_INFO jobs simultaneously

[chen <pchenthill novell com>]

On Sun, 2010-07-11 at 15:35 +0100, David Woodhouse wrote:
> On Sun, 2010-07-11 at 15:11 +0100, David Woodhouse wrote:
> > From 1d1b146e58f918f67ccff93c4fb5388429bf12e7 Mon Sep 17 00:00:00 2001
> > From: David Woodhouse <David Woodhouse intel com>
> > Date: Sun, 11 Jul 2010 15:11:17 +0100
> > Subject: [PATCH] imapx: Avoid running FETCH_NEW_MESSAGES and REFRESH_INFO jobs simultaneously
> >
> > There are various places where we interpret FETCH results and use
> > imapx_match_active_job to find the current job, which will behave badly
> > if there are two jobs which could potentially be responsible for the FETCH.
> > 
> > In particular, this was causing a problem when we triggered a fetch of new
> > messages from select_done(), and that command was submitted at the same time
> > as a refresh_info command to fetch all flags. The server (Dovecot) was
> > returning all the untagged FETCH results before either completion line,
> > and all the flags were getting "assigned" to the fetch_new_messages job,
> > causing a bunch of 'g_array_append_vals: assertion `array' failed' warnings,
> > and all messages to disappear because the refresh_info job didn't see them. 
> 
> I looked at fixing this by making the FETCH handling code work out which
> job it should be looking at.. but that's hard because of the ways in
> which a fetch_new_messages job might just be fetching flags, and a
> refresh_info job might fetch new messages.
> 
> The real fix, I think, is to stop the FETCH handling code from caring
> about the jobs at all. The untagged messages tell us about the state of
> the mailbox, and we should just deal with that information as it comes
> in, without worrying about _why_ the server is sending it.
> 
> For refresh_info to notice when messages are deleted, we could set a
> 'possibly expunged' flag in the message in our local store, then the
> FETCH handler could clear that flag whenever it gets flags for a
> message. The refresh_info logic would then be:
>  - Set this flag on all messages
>  - Issue UID FETCH 1:* (UID FLAGS)
>  - Delete all messages without this flag set.
Seems fine, this is a better approach. I don't see a problem as only one
refresh_info job can happen at a time.

> 
> The idea would be to kill off _all_ use of job->u.refresh_info.infos and
> similar fields. Where we've fetched message flags before the headers, we
> could keep a list of those in the folder info, not the job. And that
> way, _wherever_ those new flags come from (SELECT, IDLE, NOOP, FETCH,
> etc.) they'll get handled consistently.
I was just thinking whether we would need the logic to fetch new
messages from refresh_info job at-all while scanning for changes. We
fetch new messages always before scanning for changes, so ideally
scan_changes should only sync the flags.

At this point, fetching new messages from scan_changes would be used
very rarely if there was any message deleted wrongly in cache (due to
wrong  unsolicited response from server or client bug) as a fall-back
mechanism to recover those messages. So how about re-fetching those
flags in those cases (which anyway happens rarely). So this simplifies
the logic to not store uid, flags without headers for those messages in
the folder info.

> 
> All use of imapx_match_active_job() without a uid parameter would then
> be considered a bug.
Fine.

- Chenthill.