Support for rfc5465: The IMAP NOTIFY Extension #718
Conversation
|
v2: removed a very long example from the docs. It's very easy for this to fall out of sync and not really useful. |
|
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? |
|
We can also drop the incomplete imapmemserver support and enable the client-server NOTIFY tests only for dovecot. I see the I'll wait for feedback before advancing this this in any way. |
|
v3: added test for how disconnections are handled (both with and without NOTIFY). Fix |
WhyNotHugo
force-pushed
the
v2-notify
branch
2 times, most recently
from
06a1f88 to
2a6eb8e
Compare
|
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
left a comment
emersion
left a comment
There was a problem hiding this comment.
Thanks for the patch! Here are a few comments. I've only read the client part for now, not the tests nor server.
imapclient/notify.go
Outdated
| // Validate the item before encoding | ||
| if item.MailboxSpec == "" && len(item.Mailboxes) == 0 { | ||
| // Skip invalid items - this shouldn't happen with properly constructed NotifyOptions | ||
| continue |
There was a problem hiding this comment.
We should probably return an error here?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Do you prefer for this to panic or return an error?
There was a problem hiding this comment.
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.)
|
I missed the review above. Using UnilateralDataHandler is much cleaner! |
WhyNotHugo
force-pushed
the
v2-notify
branch
2 times, most recently
from
a19f92d to
91cebce
Compare
Client–server tests run only with dovecot; the imapmemserver doesn't support NOTIFY.
|
All comments addressed; ready for a second round of review. |
| case "NOMODSEQ": | ||
| // ignore | ||
| case "NOTIFICATIONOVERFLOW": | ||
| if c.options.UnilateralDataHandler.NotificationOverflow != nil { |
There was a problem hiding this comment.
Don't we need to use unilateralDataHandler() here to avoid nil pointer deref when options.UnilateralDataHandler is nil?
| cmd.pendingData.Status = data | ||
| cmd.mailboxes <- cmd.pendingData | ||
| cmd.pendingData = nil | ||
| default: |
There was a problem hiding this comment.
Nit: no need to add a default case here
| Session | ||
|
|
||
| // Authenticated state | ||
| Notify(w *UpdateWriter, options *imap.NotifyOptions) error |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I feel like the new connection tests are orthogonal to NOTIFY - if so, could these be sent as a separate PR?
Note, we had a similar test before and it ended up being racy: 805dce0
2 participants
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.