Discarded EXPUNGE/EXISTS responses

[mikecardwell]

When the wait functions return on an IdleHandle, the response doesn't contain the list of untagged EXPUNGE and EXISTS responses that have occurred. This means we don't necessarily know which sequence ids correspond to which messages anymore. E.g, we may be able to determine that a message was deleted because the exists parameter on the mailbox was decremented, but we don't know if that was the message with sequence id 1 or 10,000. This means that we have to maintain a record of the UID list and run unnecessary UID searches sometimes.

The wait functions currently return a Result<()>. They could be updated to return a list of EXPUNGE and EXISTS responses.

[jonhoo]

Yeah, that's a good point, and something I've wanted to fix for a while. I think the way we want to do this is implement Iterator for IdleHandle and yield the untagged responses as they occur, with the end of the iterator indicating that the IDLE was either cancelled or an error occurred. This is the same kind of thing we'd like to support for NOTIFY, which @jkaessens is working on over on #64, and hopefully we can kill two birds with one stone there.

[dario23]

i think we should take an even broader perspective on this, as not only idle and notify, but also a number of other imap commands (like noop, but as i read rfc3501 sec. 5.3, 5.5 and 7 basically any command) can produce out-of-band responses that we need to be able to deal with.

what do you think of having a sort of queue/iterator/channel (in golang-speak) for responses that arent related to the current command?

[jonhoo]

Hmm, yes, that sounds like it could work well. I recently had unilateral responses come up in #81, which I fixed mostly by ignoring them through 72925cf, but instead parsing them and sending them somewhere the user can observe them would be better!

[dario23]

so, while having some more thoughts on this i ended up with the following question: how unwilling are we to adopt imap-proto types in the public interface of rust-imap?

the history of ending up at that question is roughly as follows: having one iterator/channel of all unsolicited server responses depends on having one common supertype for all possible responses (which are at least EXPUNGE, RECENT, EXISTS and FETCH). rust-imap Fetch is a struct with fields for some of the possible responses, whereas imap-proto AttributeValue is an enum with some more of the possible responses, with one server response line roughly (except for body/multiline) corresponding to one enum variant. the imap-proto variants are a superset of what rust-imap Fetch contains.

async responses (e.g. from IDLE/NOTIFY responses) are usually single-line/single-item updates, not complete mailbox/fetch information "blocks" like the examine/fetch responses in rust-imap currently are.

recent/exists/expunge only carry an u32 as data, but the fetch response can be a lot of things, so to me it doesn't seem practical to duplicate types into an UnsolicitedResponse enum on the rust-imap side.

alternatively, we could do something like keeping track of all mailboxes and return complete Mailbox structs on changes; i'd see two problems with this though: we may not always have already seen all info for a mailbox (e.g. imap notify sends status updates about any folder), so we'd have to do another request for that; and secondly, if the end user is interested in a diff they have to keep copies and compare for themselves again. so all in all i think this would be rather expensive.

[jonhoo]

Hmm, I'm not entirely sure I follow. I'm not against directly exposing imap-proto types in public interfaces at all, but I would like us to ensure that users don't have to handle more responses that are impossible. Which implies that we should indeed have a UnsolicitedResponse enum of all possible unsolicited responses. I think there should also be a similar enum for IDLE/NOTIFY responses which only contains the possible responses to those. If one of those includes Fetch, that seems fine? What am I missing here?

[dario23]

I'm not against directly exposing imap-proto types in public interfaces at all, but I would like us to ensure that users don't have to handle more responses that are impossible.

If one of those includes Fetch, that seems fine? What am I missing here?

NOTIFY responses include Fetch, so we'd need that, yes.

the part i was trying to get at was that rust-imap Fetch is a struct which to construct you'd need at least a message sequence number and flags. so it doesn't fit well for single-field updates that NOTIFY sends us.

on the other hand using the imap-proto types for FETCH responses, namely AttributeValue brings (beside the dependency in our public interface) some duplication in the types, so explicitly fetching a mail gets you a rust-imap Fetch struct and an update from NOTIFY or IDLE or NOOP gets you a imap_proto::types::AttributeValue. now that i've typed it out, that doesn't sound too bad, it's two (slightly) different use-cases after all, but i think that was what i was worried about :-)

[jonhoo]

I could be wrong, but surely a NOTIFY response doesn't include all the stuff in AttributeValue, but instead only some of them? I'd propose that we create appropriate enums that have only the things you can in fact receive in each instance, and if that includes exposing an imap-proto type directly, then I'm fine with that :)

[dario23]

well, as far as i understood rfc5465 (the example on page 7 specifically), when the server sends fetch responses, any of the fetch command responses from the original imap rfc can be a part of that.

trying to test that, i didn't get dovecot to accept the more extended event sets like notify set status (selected MessageNew (uid body.peek[header.fields (from to subject)]) MessageExpunge) (subtree Lists MessageNew) from the rfc5465 example but only such sets where the parts to fetch were not specified, like NOTIFY SET STATUS (subscribed (FlagChange SubscriptionChange MessageNew MessageExpunge)).

so for now, i think i'll go for just the STATUS response and will try myself at a pull request. thanks so far :-)

[jonhoo]

Let's move further discussion to jonhoo/rust-imap#72.