Skip to content

Implementing capability caching #255

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.

Sign up for GitHub

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

Open
dequbed wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Implementing capability caching #255

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36d0c05
Implementing capability caching
dequbed Feb 6, 2023
15d3f48
Implement changes & suggestions by @jonhoo
dequbed Feb 12, 2023
04f5c9e
fix capabilities methods doclinks
dequbed Feb 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 94 additions & 2 deletions src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,11 @@ fn validate_sequence_set(
#[derive(Debug)]
pub struct Session<T: Read + Write> {
conn: Connection<T>,

// Capabilities are almost guaranteed to chance if encryption state or authentication state
// changes, so caching them in `Connection` is inappropiate.
capability_cache: Option<Capabilities>,
Copy link
Owner

@jonhoo jonhoo Feb 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that I believe that we shouldn't have the cache be in Connection. We can always just forcibly-reset it whenever the relevant underlying state changes (e.g., in login). What do you think?

Copy link
Author

@dequbed dequbed Feb 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had two arguments for not caching the capabilities in the Connection: Firstly and definitely the weaker of the two: Separation of concerns, namely doing so makes Connection more than just a very thin shim over the underlying io::{Read + Write}. Secondly and my main argument is that caching in Connection has a smaller fuck-up-protection factor for the maintainers, i.e. "us, three years older". Clearing the cache becomes an active action that needs to be thought of, and forgetting to do so is still syntactically and semantically valid code, just one that may lead to bugs in code downstream. Having the cache in Session / Client instead forces us to at least think about the cache when going from Client to Session or vice versa.

Copy link
Owner

@jonhoo jonhoo Mar 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think that's true, I also think the current split is a foot-gun because it means we have to replicate the cache state and logic in two places. If it was on Connection, it would be in one place, which means one place to make improvements / bugfixes over time. I think I'd err on the side of simplicity here, as it'll be quite rare that we add a new way to wrap Connection. And if we do, we're likely to do so by copy-pasting an existing wrapper first.


pub(crate) unsolicited_responses_tx: mpsc::Sender<UnsolicitedResponse>,

/// Server responses that are not related to the current command. See also the note on
Expand All @@ -153,6 +158,10 @@ pub struct Session<T: Read + Write> {
#[derive(Debug)]
pub struct Client<T: Read + Write> {
conn: Connection<T>,

// Capabilities are almost guaranteed to chance if encryption state or authentication state
// changes, so caching them in `Connection` is inappropiate.
capability_cache: Option<Capabilities>,
}

/// The underlying primitives type. Both `Client`(unauthenticated) and `Session`(after succesful
Expand Down Expand Up @@ -333,6 +342,7 @@ impl<T: Read + Write> Client<T> {
debug: false,
greeting_read: false,
},
capability_cache: None,
}
}

Expand All @@ -345,6 +355,47 @@ impl<T: Read + Write> Client<T> {
Ok(res)
}

/// The [`CAPABILITY` command](https://tools.ietf.org/html/rfc3501#section-6.1.1) requests a
/// listing of capabilities that the server supports. The server will include "IMAP4rev1" as
/// one of the listed capabilities. See [`Capabilities`] for further details.
///
/// This method will always bypass the local capabilities cache and send a `CAPABILITY` command
/// to the server. The [`Self::capabilities()`] method can be used when returning a cached
/// response is acceptable.
pub fn capabilities_refresh(&mut self) -> Result<&Capabilities> {
let (mut tx, _rx) = mpsc::channel();
let caps = self.run_command_and_read_response("CAPABILITY")
.and_then(|lines| Capabilities::parse(lines, &mut tx))?;
self.capability_cache = Some(caps);

self.capability_cache.as_ref()
// This path will not be hit; if the cache is not populated the above calls will either
// populate it or return with an early error.
.ok_or_else(|| panic!("CAPABILITY call did not populate capability cache!"))
}

/// The [`CAPABILITY` command](https://tools.ietf.org/html/rfc3501#section-6.1.1) requests a
/// listing of capabilities that the server supports. The server will include "IMAP4rev1" as
/// one of the listed capabilities. See [`Capabilities`] for further details.
///
/// This function will not query the server if a set of capabilities was cached, but request
/// and cache capabilities from the server otherwise. The [`Self::capabilities_refresh`] method
/// can be used to refresh the cache by forcing a `CAPABILITY` command to be send.
pub fn capabilities_ref(&mut self) -> Result<&Capabilities> {
let (mut tx, _rx) = mpsc::channel();
if self.capability_cache.is_none() {
let caps = self.run_command_and_read_response("CAPABILITY")
.and_then(|lines| Capabilities::parse(lines, &mut tx))?;

self.capability_cache = Some(caps);
}

self.capability_cache.as_ref()
// This path will not be hit; if the cache is not populated the above `if` will either
// populate it or return with an early error.
.ok_or_else(|| panic!("CAPABILITY call did not populate capability cache!"))
}

/// Log in to the IMAP server. Upon success a [`Session`](struct.Session.html) instance is
/// returned; on error the original `Client` instance is returned in addition to the error.
/// This is because `login` takes ownership of `self`, so in order to try again (e.g. after
Expand Down Expand Up @@ -502,6 +553,7 @@ impl<T: Read + Write> Session<T> {
let (tx, rx) = mpsc::channel();
Session {
conn,
capability_cache: None,
unsolicited_responses: rx,
unsolicited_responses_tx: tx,
}
Expand Down Expand Up @@ -771,12 +823,52 @@ impl<T: Read + Write> Session<T> {
self.run_command_and_check_ok(&format!("UNSUBSCRIBE {}", quote!(mailbox.as_ref())))
}

/// The [`CAPABILITY` command](https://tools.ietf.org/html/rfc3501#section-6.1.1) requests a
/// listing of capabilities that the server supports. The server will include "IMAP4rev1" as
/// one of the listed capabilities. See [`Capabilities`] for further details.
///
/// This method will always bypass the local capabilities cache and send a `CAPABILITY` command
/// to the server. The [`Self::capabilities()`] method can be used when returning a cached
/// response is acceptable.
pub fn capabilities_refresh(&mut self) -> Result<&Capabilities> {
let caps = self.run_command_and_read_response("CAPABILITY")
.and_then(|lines| Capabilities::parse(lines, &mut self.unsolicited_responses_tx))?;
self.capability_cache = Some(caps);

self.capability_cache.as_ref()
// This path will not be hit; if the cache is not populated the above calls will either
// populate it or return with an early error.
.ok_or_else(|| panic!("CAPABILITY call did not populate capability cache!"))
}

/// The [`CAPABILITY` command](https://tools.ietf.org/html/rfc3501#section-6.1.1) requests a
/// listing of capabilities that the server supports. The server will include "IMAP4rev1" as
/// one of the listed capabilities. See [`Capabilities`] for further details.
///
/// This function will not query the server if a set of capabilities was cached, but request
/// and cache capabilities from the server otherwise. The [`Self::capabilities_refresh`] method
/// can be used to refresh the cache by forcing a `CAPABILITY` command to be send.
pub fn capabilities_ref(&mut self) -> Result<&Capabilities> {
if self.capability_cache.is_none() {
let caps = self.run_command_and_read_response("CAPABILITY")
.and_then(|lines| Capabilities::parse(lines, &mut self.unsolicited_responses_tx))?;

self.capability_cache = Some(caps);
}

self.capability_cache.as_ref()
// This path will not be hit; if the cache is not populated the above `if` will either
// populate it or return with an early error.
.ok_or_else(|| panic!("CAPABILITY call did not populate capability cache!"))
}

/// The [`CAPABILITY` command](https://tools.ietf.org/html/rfc3501#section-6.1.1) requests a
/// listing of capabilities that the server supports. The server will include "IMAP4rev1" as
/// one of the listed capabilities. See [`Capabilities`] for further details.
pub fn capabilities(&mut self) -> Result<Capabilities> {
self.run_command_and_read_response("CAPABILITY")
.and_then(|lines| Capabilities::parse(lines, &mut self.unsolicited_responses_tx))
// TODO: This emulated the same behaviour as before, with each call issuing a command.
// It may be sensible to allow hitting the cache here.
self.capabilities_refresh().map(|caps| caps.clone())
}

/// The [`EXPUNGE` command](https://tools.ietf.org/html/rfc3501#section-6.4.3) permanently
Expand Down
22 changes: 22 additions & 0 deletions src/types/capabilities.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use imap_proto::{Capability, Response};
use ouroboros::self_referencing;
use std::collections::hash_set::Iter;
use std::collections::HashSet;
use std::fmt;
use std::sync::mpsc;

const IMAP4REV1_CAPABILITY: &str = "IMAP4rev1";
Expand Down Expand Up @@ -43,6 +44,17 @@ pub struct Capabilities {
pub(crate) capabilities: HashSet<Capability<'this>>,
}

impl Clone for Capabilities {
fn clone(&self) -> Self {
// Give _rx a name so it's not immediately dropped. Otherwise any unsolicited responses
// that would be send there will return a SendError instead of the parsed response simply
// being dropped later.
let (mut tx, _rx) = mpsc::channel();
Self::parse(self.borrow_data().clone(), &mut tx)
.expect("failed to parse capabilities from data which was already successfully parse before")
}
}

impl Capabilities {
/// Parse the given input into one or more [`Capabilitity`] responses.
pub(crate) fn parse(
Expand Down Expand Up @@ -98,3 +110,13 @@ impl Capabilities {
self.borrow_capabilities().is_empty()
}
}

impl fmt::Debug for Capabilities {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let mut dbg = f.debug_tuple("Capabilities");
for x in self.borrow_capabilities() {
dbg.field(x);
}
dbg.finish()
}
}