Re: [Evolution-hackers] imapx: Avoid running FETCH_NEW_MESSAGES and REFRESH_INFO jobs simultaneously
On Mon, 2010-07-12 at 11:32 +0530, chen wrote: > > 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 Or if messages come in between the call to fetch_new_message() and the later call to scan_changes(). And in fact there are a few other places that this can happen and we can get flags for messages we've never heard about -- it happens in IDLE too, or on NOOP, and in SELECT when we're using QRESYNC. I was thinking of having a per-folder list of 'messages for which we only have flags', and all those places can just append to it. And something, somewhere, will check for it and trigger a fetch of the headers. But it still won't care *why* we were told ;) -- dwmw2 ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] imapx: Avoid running FETCH_NEW_MESSAGES and REFRESH_INFO jobs simultaneously
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 > > 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
Re: [Evolution-hackers] imapx: Avoid running FETCH_NEW_MESSAGES and REFRESH_INFO jobs simultaneously
On Sun, 2010-07-11 at 15:11 +0100, David Woodhouse wrote: > From 1d1b146e58f918f67ccff93c4fb5388429bf12e7 Mon Sep 17 00:00:00 2001 > From: David Woodhouse > 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 WoodhouseOpen Source Technology Centre david.woodho...@intel.com Intel Corporation ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... http://mail.gnome.org/mailman/listinfo/evolution-hackers
