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

Implementing capability caching #255

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

Conversation

dequbed
Copy link

@dequbed dequbed commented Feb 6, 2023

Based on this comment by @jonhoo on #243 this PR adds the ability for Client and Session to cache returned Capabilities.

Future work should try to parse (the optionally sent) Capabilities from untagged responses to EHLO, STARTTLS, and AUTHENTICATE automatically; if none are sent they should be automatically queried.


This change is Reviewable

@dequbed dequbed mentioned this pull request Feb 6, 2023
@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

Merging #255 (04f5c9e) into main (41c5597) will decrease coverage by 0.8%.
The diff coverage is 33.8%.

Additional details and impacted files
Impacted Files Coverage Δ
src/client.rs 91.5% <26.0%> (-1.7%) ⬇️
src/types/capabilities.rs 55.9% <56.2%> (+0.1%) ⬆️

src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/types/capabilities.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved

// 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

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

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

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.

src/client.rs Outdated Show resolved Hide resolved
@dequbed dequbed requested a review from jonhoo February 19, 2023 17:09
@jonhoo
Copy link
Owner

jonhoo commented Mar 11, 2023

I think the last bit here now is #255 (comment)

@bogthe
Copy link

bogthe commented Apr 7, 2024

Hi @dequbed, I really liked that you were adding support for SASL in #243. Do you have any plans to update this PR? If you're short on time, I would be happy to help getting this (and that one) merged!

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

Successfully merging this pull request may close these issues.

3 participants