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.woodho...@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. _______________________________________________ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers