Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Freeze" the view of sequence numbers on per-connection basis #318

Closed
foxcpp opened this issue Dec 7, 2019 · 5 comments
Closed

"Freeze" the view of sequence numbers on per-connection basis #318

foxcpp opened this issue Dec 7, 2019 · 5 comments
Assignees
Labels

Comments

@foxcpp
Copy link
Collaborator

foxcpp commented Dec 7, 2019

The backend does not have a consistent view of the per-connection information and I think in the end it is a good thing since it reduces the backend implementation complexity considerably (I think \Recent support is not worth the changes I proposed in #260). But the outcome is that backend cannot implement the updates buffering themselves so it needs to be done in go-imap.

Here is what I think needs to be done:

  • Whenever an ExpungeUpdate is received from the backend, it is stored in the per-connection buffer without being sent to the client.
  • Whenever the client issues a command using sequence numbers, they are translated to be consistent with how the backend sees them (decrement for each buffered EXPUNGE with a sequence number smaller than the number in the command).
  • Attempts to access expunged messages fail in one of the ways outlined in https://tools.ietf.org/html/rfc2180#section-4. The preferable and the most reliable way is to cache metadata and send it to the client (but fail the body access). This seems to be the way Dovecot works according to https://www.imapwiki.org/ImapTest/ServerStatus.
  • IDLE disables buffering logic resulting in EXPUNGE responses being sent immediately.

Commands that flush sequence numbers buffer:

  • EXPUNGE
  • CHECK (command explicitly meant for this)
  • NOOP (historically also used for the same purpose as CHECK)
  • IDLE (needs exported BufferExpunge(bool) to be called from go-imap-idle)

Commands that discard sequence numbers buffer without sending updates to the client:

  • Commands changing the active mailbox (SELECT, EXAMINE, CLOSE)
  • Commands changing the state of user authentication (LOGOUT, UNAUTHENTICATE)

Was briefly discussed in #300. The current behavior is racy/unsafe and non-RFC conforming since RFC 3501 prohibits EXPUNGE responses to be sent as a response to the FETCH command but we send updates at any time.

Reference: https://tools.ietf.org/html/rfc2180

@foxcpp foxcpp added the server label Dec 7, 2019
@foxcpp
Copy link
Collaborator Author

foxcpp commented Dec 7, 2019

I will experiment with this and see what else needs to be done.

@foxcpp
Copy link
Collaborator Author

foxcpp commented Dec 14, 2019

no longer relevant, see latest comment

Started working on it. I think there is also a need to buffer lastest MailboxUpdate and send it in response to the next command. I am not sure if this is required by RFC 3501, but this matches Dovecot behavior and has a higher chance of being interoperable.

@emersion, is API between go-imap and extensions covered by 1.0 stability? While not directly breaking anything, extensions that accept or return sequence numbers may (not sure, haven't looked into it yet) need to be aware of it and call corresponding functions (SeqForBackend and SeqForClient) on them.

@foxcpp
Copy link
Collaborator Author

foxcpp commented Dec 14, 2019

no longer relevant, see latest comment

... While not directly breaking anything, extensions that accept or return sequence numbers may (not sure, haven't looked into it yet) need to be aware of it and call corresponding functions (SeqForBackend and SeqForClient) on them.

Backward-compatible option is to keep current behavior (totally broken) default and enable updates buffering via Server flag. I would really like to avoid introducing sub-optimal design choices to maintain compatibility while also enabling important fixes. There are more options, such as having buffering enabled only if extensions are marked as "compatible". Note sure what to do here.

@foxcpp
Copy link
Collaborator Author

foxcpp commented Dec 15, 2019

No, the idea is flawed. There is a need to enforce invariant "no expunges should happen while commands are in progress". Only backend can properly enforce this.
So it is not possible to implement this without cooperation with backend, exploring options for such integration...

More specifically, SeqForBackend and storage access should be fused into a single serializable operation without EXPUNGE happening in the middle.

I think updates dispatching should be moved into backend code, then made aware of connections and mailboxes state (see #260 comments). This will allow it to perform any translations on its own, using serialization mechanisms appropriate for used strorage.

@foxcpp foxcpp modified the milestone: v2 Dec 15, 2019
@foxcpp
Copy link
Collaborator Author

foxcpp commented Dec 15, 2019

Closing this, I will open a separate issue with proposed Backend interface changes for go-imap v2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant