Bug 667725 - imapx_untagged: code should not be reached
authorDavid Woodhouse <David.Woodhouse@intel.com>
Mon, 21 May 2012 15:45:56 +0000 (16:45 +0100)
committerDavid Woodhouse <David.Woodhouse@intel.com>
Mon, 21 May 2012 15:45:56 +0000 (16:45 +0100)
commit17f3fa1b12faa89158458d976c110cc9f8733a56
tree2a745b7ebc44fcd36a7651c582857cc030f9290f
parent7dd91af4568d1247e32e33319a875c274d6c7ca5
Bug 667725 - imapx_untagged: code should not be reached

This code is evil.

When we scan a folder for new messages, we issue a 'FETCH 1:* (UID FLAGS)'
or similar command.

When we receive an untagged FETCH from the server telling us flags for a
message, we make a decision about whether that information was solicited
by such a command, or whether it was unsolicited.

If it was unsolicited, we process it normally as an asynchronous flags
update and all is well.

If it was solicited, we add the UID to a list. When the FETCH (UID FLAGS)
command *completes*, we'll sort that list and then fetch the full headers
for each message.

However, we weren't very good at telling when an update was solicited.
Assuming that only solicited messages will have a UID is bogus.

This was failing if an unsolicited update came in when the (UID FLAGS)
fetch had completed, and we were already fetching the message headers.
The "new" UID would be added to the end of the list, even if we were
already fetching that message or if we already had it in cache. We'd
issue a FETCH command for it, and the barf when the server complied,
because when the UID list wasn't sorted we wouldn't find the offending
uid when we looked for it.

The simple "fix" for this is to keep a boolean flag 'scan_changes' which
is TRUE only when that FETCH (UID FLAGS) command is running. If a flags
change comes in at any other time, it is definitely unsolicited and
should *not* be added to the uidset. This at least protects us from
having UIDs added after we've sorted the list and started to do other
things with it, which was causing the crash.

In fact, this whole 'solicited' vs. 'unsolicited' thing is a design
mistake. In imapx_untagged() we should never care about what we asked
for and what we didn't. That's why the responses are *untagged*. The
server tells us things about the state of the mailboxes, and we should
process that information into our own local cache — it shouldn't
*matter* what we asked for. But that's a more intrusive fix for another day.

In addition, we were reliably *triggering* this behaviour in some cases
because we had to issue a SELECT for the folder in question before
issuing the FETCH (UID FLAGS) command. And on completion of the SELECT,
if UIDNEXT had increased, we were automatically issuing a *new* FETCH
(UID FLAGS) command starting from the last-known-uid in our cache. This
was entirely gratiutous, so use the same scan_changes boolean flag to
avoid it in that situation.
camel/camel-imapx-server.c