Support for rfc5465: The IMAP NOTIFY Extension

[WhyNotHugo]

This patch series implements support for the NOTIFY extension.

The first commit implements support in imapclient, the main focus of my work. I've been using this on a client to monitor changes with success.

The second commit implements support in imapserver.

The third commit adds a minimal scaffold implementation in imapmemserver. This implementation simply rejects any request (which it technically allows as per the spec). It's not an actually useful implementation, and is mostly there so we can have minimal client-server unit tests. Implementing proper NOTIFY support for imapmemserver likely requires substantial changes which fall beyond the scope of this series.

This is likely easiest to review on a per-commit basis, and about half of the LoC are just unit tests for encoding/decoding.

[WhyNotHugo]

v2: removed a very long example from the docs. It's very easy for this to fall out of sync and not really useful.

[WhyNotHugo]

Tests with dovecot failed. When writing these tests I didn't realise that they also run with another server in CI.

I lean towards dropping the third commit— the one which implements the scaffold for NOTIFY in imapmemserver (along with the test which run server+client).

Thoughts?

[WhyNotHugo]

We can also drop the incomplete imapmemserver support and enable the client-server NOTIFY tests only for dovecot. I see the useDovecot flag which should be useful for that.

I'll wait for feedback before advancing this this in any way.

[WhyNotHugo]

v3: added test for how disconnections are handled (both with and without NOTIFY). Fix Overflow() channel hanging on disconnection.

[WhyNotHugo]

I've re-written the client test to run only with dovecot, which has real NOTIFY support. Those test will be skipped with the imapmemserver, but are a lot more meaningful than before.

I've ripped out the scaffold for the imapmemserver's NOTIFY support. Such broken support was useless beyond these tests.

[emersion]

Thanks for the patch! Here are a few comments. I've only read the client part for now, not the tests nor server.

[emersion]

We should probably return an error here?

[WhyNotHugo]

I can't find any other method which uses an encoder and returns an error.

Is it fine to leave the command half-written in this case? (this indicates a programmer error anyway).

[emersion]

For some commands we panic when fed an invalid state (e.g. see GetMetadata). For some commands we validate upfront and we return a command struct already completed with an error (e.g. see Enable). Latter is more complicated and better suited for cases where the failure doesn't indicate a programmer error (Enable is probably a bad usage for this).

[WhyNotHugo]

Do you prefer for this to panic or return an error?

[emersion]

I would prefer to not return an error so that callers can chain .Wait() like other short-running commands. I don't have strong opinions about returning a command struct already completed with an error vs. a panic. Simplest would probably be to panic. (We can change it later if it turns out to be an annoyance.)

[emersion]

It seems like this comment has been addressed?

[WhyNotHugo]

I've added another commit (which probably needs to be squashed later) which allows recognising whether a NOTIFY stream ended because of an explicit NOTIFICATIONOVERFLOW or because the connection closed.

I missed the review above. Using UnilateralDataHandler is much cleaner!

[WhyNotHugo]

All comments addressed; ready for a second round of review.

[emersion]

I wonder about the implications of passing an UpdateWriter to the backend here. Up until now backends would only use UpdateWriter synchronously in handlers. Here the backend would need to use it after Notify returns. We have conn.encMutex to avoid stepping on our own toes when sending an update, but that still leaves considerations such as sequence number issues with EXPUNGE notifications while another command is running, see section 5.5.

Maybe there is a way to expose a safer interface to backends. Maybe we should just leave it up to the backend and document the issue.

[WhyNotHugo]

Would you prefer that I drop the imapserver support from this PR and focus on the client part first?

[WhyNotHugo]

FWIW, I only wrote some of the server portions to test the client, but eventually moved on to just testing with dovecot.

[emersion]

Moving the server parts to a separate PR would allow the client part to be merged first, which sounds like a good idea.

[emersion]

Shouldn't we wait for the command to complete in all tests?

[WhyNotHugo]

Client.Notify calls cmd.Wait() internally. But as I re-think the approach, that's probably a mistake and it shouldn't, correct?

[arnt]

Wow, cool, support for 5465!

I find it difficult to assess the test coverage here, perhaps because of the verbosity. @emersion suggests tables, and I actually wrote the examples in the RFC together with unit tests that might be described as tables. That was C++, in Go I think I'd use an array of 5-tuples:

• a function literal to set up notifications
• a string with the expected/correct IMAP command
• a string with a mailbox to select
• a string that is sent by a mock server after the select has completed
• a function literal that asserts that go-imap acts correctly on the mock server string

Each example in section 5 corresponds to one of those tests, and that gave full coverage, easily verifiable. (Rereading now, I notice that the interaction between 5267 and 5465 isn't described in this way. Sigh. Sorry. I didn't have 5267 support myself and I suppose I didn't pay attention.)

Second, I like the use of unilateralDataHandler, but feel that it misses something. It can be nil, but handling unilateral data is almost mandatory now. (If gmail sends you an unsolicited BYE, that generally just means that a server there is overloaded and you should reconnect and be routed to another.) Nil seems to be a poor default, but I can't suggest a good way to write a general handler in Go… sorry about the lack of clarity here.

[WhyNotHugo]

Tests were verbose, but I originally preferred to keep them that way since they're at least straightforward without too many clever abstractions.

TableDrivenTests is much shorter and even simpler to read, so migrated to that format.