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

Delegate updates dispatching to the backend #323

Closed
3 tasks
foxcpp opened this issue Dec 15, 2019 · 3 comments
Closed
3 tasks

Delegate updates dispatching to the backend #323

foxcpp opened this issue Dec 15, 2019 · 3 comments
Labels
breaking Backward-incompatible changes needed server
Milestone

Comments

@foxcpp
Copy link
Collaborator

foxcpp commented Dec 15, 2019

Problem

  • It is unsafe to send updates to clients literally "at any time". This causes major problems with synchronization and sequence numbers.
  • RFC 3501 prohibits sending EXPUNGE updates in response to FETCH and some other commands.

Previous discussions:

Proposed solution

I want to propose to redesign Backend interface in a way that completely moves update handling to the backend side. This includes EXPUNGE updates buffering and sequence numbers translation. This adds additional flexibility in implementation. For example, a backend using MVCC-enabled database can rely on it to provide consistent messages view for clients. Utilities that simplify implementations of rudimentary/naive backends can be added to backendutil package.

As a starting point, here is the snippet with a bunch of Go interfaces:

type Conn interface {
  SendUpdate(u backend.Update)
}

type Backend interface {
  Login(conn Conn, username, password string) (Session, error)
}

type Session interface {
  OpenMbox(mbox string, readOnly bool) (Mailbox, error)
}

type Mailbox interface {
  ...
  Close() error
}

Subtasks

  • Remove existing updates handling code from go-imap/server
  • Define Conn interface for backend and update Backend interfaces
  • Implement utilities for simple backends
@foxcpp foxcpp added server breaking Backward-incompatible changes needed labels Dec 15, 2019
@foxcpp foxcpp added this to the v2 milestone Dec 15, 2019
@foxcpp foxcpp mentioned this issue Dec 15, 2019
@emersion
Copy link
Owner

  • It would be nice to have a list of when we're allowed to send each type of unilateral update
  • When should the backend know whether it's a good time to send an unilateral update? (e.g. how should the backend know the client issued a NOOP and it needs to flush its unilateral update buffer)
  • I see some overlap between Conn and Session. I wonder if there's a way to simplify this (e.g. with something like Server.SendUpdate(s Session, u backend.Update)).
  • I'd really like a collection of backendutils helpers to handle all of this. I don't expect backend authors to know everything about IMAP.

@foxcpp
Copy link
Collaborator Author

foxcpp commented Dec 18, 2019

It would be nice to have a list of when we're allowed to send each type of unilateral update

AFAIK, will check RFCs later:

  • FLAGS, EXISTS, RECENT can be sent in response to any command with a selected mailbox.
  • EXPUNGE cannot be sent in response to pipeline-able commands using sequence numbers (that includes STORE, FETCH but not UID FETCH, UID STORE and COPY, later is not permitted to be pipelined).
  • For interoperability purposes, it is a good idea to not send updates between commands (after the tagged response to the previous command, but before the next command is received). We cannot expect client to actually consume them and take into account when sending the next command.
  • Previous statement applies for EXISTS update too. It will affect the effect of STORE 1:* +FLAGS (\Deleted), for example.

When should the backend know whether it's a good time to send an unilateral update? (e.g. how should the backend know the client issued a NOOP and it needs to flush its unilateral update buffer)

Mailbox.Poll() is called by go-imap on NOOP with a selected mailbox.

Defined in backend/:

type MailboxPoller interface {
	// Poll requests mailbox updates.
	Poll() error
}

I see some overlap between Conn and Session. I wonder if there's a way to simplify this (e.g. with something like Server.SendUpdate(s Session, u backend.Update)).

Server would have to find the right connection to use. This also needs to be properly synchronized to prevent races. I expect backend.Conn to be directly implemented by *server.conn (possibly with safeguards to prevent buggy backends from breaking something).

I'd really like a collection of backendutils helpers to handle all of this. I don't expect backend authors to know everything about IMAP.

Fair point. I had a very incomplete understanding of IMAP when I started working on go-imap-sql.

Need to think, though, what is actually trivial for backend to implement and what needs to have a backendutil helper.

Ideas proposed in #318 can be implemented using an object called SeqMapper, I guess. Though it needs to be used with care to avoid race conditions.

Here are considerations for SeqMapper usage. I think they should be included in documentation, because it is going to be really easy to use incorrectly and introduce subtle data-loss bugs.

  • No EXPUNGEs should happen while the command using sequence numbers is running or backend should otherwise guarantee a stable mailbox view in the presence of parallel EXPUNGEs. That poses additional challenge for implementations that permit external software to remove messages without the server coordination (sigh maddyctl).
  • SeqMapper operations should be executed together with storage access in the atomic way (e.g. while holding the storage lock).

@emersion
Copy link
Owner

emersion commented Apr 4, 2023

Done in go-imap v2.

@emersion emersion closed this as completed Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Backward-incompatible changes needed server
Projects
None yet
Development

No branches or pull requests

2 participants