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

[David Woodhouse <dwmw2 infradead org>]

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.

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.

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

Chen, what do you think?

-- 
David Woodhouse                            Open Source Technology Centre
David Woodhouse intel com                              Intel Corporation