NOTIFY extension and asynchronous events

[jkaessens]

I'm currently looking into writing a tool to monitor mailboxes for status changes. The typical way for this is probably SELECT/IDLE but I have quite a lot of mailboxes that I'd like to check. I could get a list of all mailboxes and check their statuses periodically but when I'm on a low-bandwidth connection with unfortunate and expensive mobile plans, that would also be a bad idea.
Therefore, I would like to use my server's NOTIFY extension (as in RFC5465). After authenticating, I'd issue a notify request for a set of events on a set of mailboxes, that could look like this:
a NOTIFY SET (personal (MessageNew MessageExpunge))
The server responds with a OK NOTIFY completed. (at least Dovecot does). After some time, additional (untagged) responses are generated asynchronously, i.e.

* STATUS Sent (MESSAGES 1082 UIDNEXT 4821 UNSEEN 0)
* STATUS INBOX (MESSAGES 307 UIDNEXT 13311 UNSEEN 1)

until I either disconnect or stop them by issuing NOTIFY NONE.

Well, the notification request can be sent by the current API. However, I don't see a way to collect those untagged status reports. One problem might be that they may be scattered through the whole connection like that (C being the client, S the server):

C: a NOTIFY SET (personal messagenew messageexpunge)
S: a OK NOTIFY completed.
C: a NOOP
S: * STATUS Foo (...)
S: a OK NOOP completed.
S: * STATUS Bar (...)

I haven't looked into the depths of rust-imap but I guess that it could probably interfere with the response parsing. My uneducated guess would be that implementing something like the IdleHandle where all those STATUS messages are collected that are not a direct response to a STATUS command. That might be hard to detect, though, if at all possible.

Do you have any ideas how to support this type of operation?

[jonhoo]

That's an interesting feature... The way we do this using IDLE is that you cannot simultaneously issue any other requests while an IDLE is ongoing, so every line sent by the server is necessarily a response to the IDLE command. It sounds like that might be how we want to handle NOTIFY too. Maybe have the thing returned by Client::notify() implement Iterator so that you can iterate over all the events directly? It'd probably be inconvenient not to be able to also issue additional commands in response to a STATUS though :/

One question here is how the spec says that these untagged responses should be handled? What if you run both IDLE and NOTIFY, or two NOTIFYs -- how do you assign an untagged response line to the corresponding command?

[jkaessens]

That's an interesting feature... The way we do this using IDLE is that you cannot simultaneously issue any other requests while an IDLE is ongoing, so every line sent by the server is necessarily a response to the IDLE command. It sounds like that might be how we want to handle NOTIFY too. Maybe have the thing returned by Client::notify() implement Iterator so that you can iterate over all the events directly? It'd probably be inconvenient not to be able to also issue additional commands in response to a STATUS though :/

Yes. When my NOTIFY request signals a new message in a specific mailbox, I would like to FETCH it whenever I want, without cancelling the NOTIFY and resuming it afterwards. This could also lead to lost updates in other mailboxes as no notifications would be queued up.

One question here is how the spec says that these untagged responses should be handled? What if you run both IDLE and NOTIFY, or two NOTIFYs -- how do you assign an untagged response line to the corresponding command?

For your second question, there is at most one NOTIFY active at any time. If you send another NOTIFY while one is already active, it is updated with the new parameters. For the handling of NOTIFY and IDLE at the same time, the specs read:

If IDLE [RFC2177] (as well as this extension) is supported, then
while processing any IDLE command, the server MUST send exactly the
same events as instructed by the client using the NOTIFY command.

That makes NOTIFY a superset of IDLE. So, when the server supports both, all IDLEs could be converted to NOTIFY and the IdleHandle could receive any untagged responses that could possibly originate from a NOTIFY or IDLE while the "regular" client handle receives only tagged or at least solicited responses. On the other hand, I would expect the server to actually execute IDLE when calling client::idle(). Hmmm.

[jonhoo]

Hmm, yeah, that makes this a little tricky given the way the crate's code is currently set up. Specifically, the NotifyHandle would need to hold a mutable reference to the Client (to read more lines), while the rest of the code is also allowed to keep a mutable reference. The code as currently written is heavily geared towards one-at-a-time command processing, and does not deal well with untagged responses at all. I'd like to change this at some point, but it is a larger rewrite from the code that @mattnenterprise originally wrote.

I don't know how/if tokio-imap deals with this at the moment, but maybe @djc has some useful insights we could carry over? I suspect that, in time, tokio-imap and this crate will converge, and I'm hesitant to completely re-write this crate's internals for this feature, at least without a very concrete proposal for how that'd be done. If you have some time to tinker with it, I'd be happy to review a PR, but for now I'll leave this ticket open with no concrete plan to fix. Sorry about that. I'll keep it in the back of my mind though for if I come up with a good strategy for supporting it!

[djc]

As far as I understand the (IMAP4rev1) protocol, you cannot really run multiple commands at once. I haven't really looked into the IDLE and NOTIFY extensions, though, so I'm unfamiliar with how exactly they do things. It looks like the way it would have to work is that any response might include STATUS messages, and then the client caller will just have to handle that. tokio-imap streams each response as it comes in, so the receiver would have to deal with it. At least in tokio-imap, my design choices have really revolved around being a thin layer that implements the protocol, so I would not include any higher-level abstractions that try to make things more convenient, at the risk of the abstractions being leaky.

[jonhoo]

As far as I understand the (IMAP4rev1) protocol, you cannot really run multiple commands at once.

Yeah, that's the impression I had too, which is why IDLE is implemented the way it is in rust-imap at the moment. It's interesting that IDLE/NOTIFY work so differently. @jkaessens perhaps a way to work around this is to open two connections? One to subscribe and one to issue commands. If we did it that way, NOTIFY could be implemented pretty much exactly like IDLE.

At least in tokio-imap, my design choices have really revolved around being a thin layer that implements the protocol, so I would not include any higher-level abstractions that try to make things more convenient, at the risk of the abstractions being leaky.

I think that's probably a good idea, though with rust-imap I believe @mattnenterprise's intention was to provide a slightly higher-level interface. And I think that can be good for some use-cases too. Maybe one day we'll build rust-imap on top of tokio-imap :)

[jkaessens]

perhaps a way to work around this is to open two connections? One to subscribe and one to issue commands. If we did it that way, NOTIFY could be implemented pretty much exactly like IDLE.

Actually, many IMAP clients do this. In my IMAP server logfiles, I usually see clients connecting multiple times, i.e. Mozilla Thunderbird's default is 5, and some Android ones open a new connection for every mailbox to be watched (those "push" or "sync" functions).
As for me, using a second connection solely for NOTIFY would be a very nice tradeoff. Even idle connections with the occasional keep-alive don't consume as much volume as mailbox polling (supposedly).

[jonhoo]

Okay, so in that case it seems reasonable for us to do this the same way we do IDLE — .notify() takes an &'a mut self, and returns a NotifyHandle<'a> that holds the mutable reference to the Client until you end it. And if you want to also issue commands, that has to be done on a separate connection?

[jonhoo]

@djc is there currently support in imap-proto for parsing untagged STATUS responses like:

S: * STATUS Foo (...)

[jkaessens]

Other than IDLE, NOTIFY requires a potentially large set of arguments so it would also need a &str parameter. But yes, that would work.

[jonhoo]

Oh, yes, sorry, I left out the arguments for clarity. I think I'd probably make a NOTIFY be issued using either a builder pattern or by giving a struct. Or maybe both, depending on just how complex the arguments are.

[djc]

@jonhoo there is now.

[jonhoo]

I don't think I'll end up using it in rust-imap, but would it be worthwhile for completeness to also add support for the IDLE responses? From what I can tell, the only valid response lines are:

* <msgnum> EXPUNGE
* <msgnum> EXISTS
+ idling

[jonhoo]

@jkaessens Given that it looks like this can be implemented pretty straightforwardly, you think you might want to write up a PR? I'd be happy to mentor and review. Basically, start by copying the code for doing IDLE, and then implement Iterator for the NOTIFY struct which keeps parsing the lines read from the connection and parsed using imap-proto's parse_response. This will (once @djc cuts a new imap-proto release) produce a MailboxDatum::Status, which you can then return (using the ZeroCopy struct introduced in #58). I propose you start out on top of #58, as that cleans up a bunch of other things in the code too!

[jkaessens]

Alright, let's try that! It will take me some days to crawl through the code, though.

[jonhoo]

@jkaessens great! Happy to help give you pointers if you need them! https://gitter.im/djc/tokio-imap may also be a useful point of contact.

[jkaessens]

@jonhoo So the NOTIFY syntax is basically NOTIFY <NONE | [STATUS] SET <mailbox-spec> >. If it's a NONE or a non-STATUS SET, the server just returns an OK response. If the STATUS flag is set, it returns one STATUS line for every mailbox specified before the final OK. I'm thinking about how to implement the Iterator::next. If NONE or non-STATUS SET, it's trivial. But what to do when it's STATUS-flagged? I have two ideas for that:

• Buffer all intermediate STATUS lines in the initial command response, store them into an internal vector and return the OK response. The next would then check whether the vec is empty and either pop an element from the vec or block on reading from the connection
• Not buffering all the STATUS lines. Then, it would also need to defer evaluation of the actual command status and the caller wouldn't know if the command succeeded. The iterator would then return NO or BAD on the first next call or return the STATUS messages with a mixed-in OK response.

The first one would be a little less efficient than the second one while the second one a lot more inconsistent than the first one. And as we're talking about TCP connections, that Vec::is_empty() call would certainly not cause any bottlenecks. I'm quite sure the first one would be the better solution but it still.... doesn't feel right. Any suggestions?

[jonhoo]

I'm inclined to have two different methods then:

fn notify(...) -> NotifyHandle {}
fn status_and_notify(...) -> (Vec<Status>, NotifyHandle) {}

[jkaessens]

@jonhoo Please take a look at https://github.com/jkaessens/rust-imap/tree/notify . This includes your PR but when trying to use the ZeroCopyResult, the borrow checker started to question my mental stability. Let's just focus on feedback on my last few commits. There's also a #[test] for it.

[jonhoo]

@jkaessens Overall it looks pretty reasonable! I wouldn't worry too much about ZeroCopyResult. The biggest thing I would change is to have Client::notify() take a NotifyOp directly and immediately issue the NOTIFY command when called. I'd then also add a second method to Client, Client::notify_status, which also takes a NotifyOp and returns what NotifyHandle::set_status currently does. This saves the user from having to call c.notify().set(op) or c.notify().set_status(op), and instead exposes those directly on c.

On a separate point, isn't the idea of NOTIFY that it gives you a stream of notification events? That's not something that's exposed in the current code, unless I'm missing something?

Some minor points:

• if cmd.items.len() == 0 should be if cmd.items.is_empty(). There are some other cases where this change could be made too.
• for i in cmd.items.iter() -> for i in &cmd.items
• unless I'm missing something, all methods could take &NotifyOp
• NotifyOp should not be tied to 'a, as that would imply the data in the NotifyOp must outlive the client handle passed to NotifyHandle::new, which isn't necessary. In fact, given that we don't keep the values from NotifyOp beyond set/set_status, you can just leave off the lifetime there entirely to imply that any lifetime is acceptable.
• for f in status.into_iter() I believe can be for f in status

[jonhoo]

Also, I think it's probably reasonable to open a PR for your changes -- it makes it a little easier to discuss/comment inline. We can deal with rebasing when #58 is merged when that happens.

[jkaessens]

@jonhoo Thanks a lot for your feedback. Being a Rust newbie, I'm still struggling to leave my burnt-in C++'isms behind and adopt The Rust Way to do things. As you have certainly noticed, it currently revolves around understanding when to use &str with an enforced lifetime or String, when to give a &Vec<u8> or a &[u8], etc etc.
However, I will try to refine the code with your suggestions. Also, I didn't file a PR because my code part does not actually do what the PR would promise. It still does not provide a method for reading from the STATUS stream and it does not currently keep the connection alive.

[jonhoo]

That's fine -- you can just put [WIP] in the PR title so we know not to merge it yet. That way I can monitor the progress and give you suggestions along the way :)

Another useful tool is clippy -- it gives suggestions for how to make your code more idiomatic! I don't know that it's been run on the imap crate as a whole though, so you may need to filter out some noise.

[jonhoo]

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

[freswa]

@jkaessens Are you still interested in contributing this code? Would you mind if someone else continues your work?

[dario23]

@freswa the project sort-of moved, see @jonhoo comment above. any new development is at the jonhoo/rust-imap fork only.

[freswa]

@dario23 I'm aware of that. Though, the NOTIFY extension has not been implemented yet and this is the only approach for jonhoo/rust-imap I could find. I'd like to ask the author first before I start another approach on the recent repo.

[jkaessens]

@freswa Thanks for asking. I followed the project quite a while but lost track when there were some movements towards tokio-style workflow. I'm quite busy right now but may pick this up in the future. If someone wants to work on that, I'd be happy to take part in it/assist. So yes, feel free to do whatever you like with that, and I'd might chime in at some point, or even provide test infrastructure for NOTIFY.