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

Remove the Session cache from the cryptostore implementations #3799

Merged
merged 4 commits into from
Aug 6, 2024

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Aug 2, 2024

This PR is another step in the #2624 epic. It moves the Olm Session cache from each individual cryptostore implementation to the CryptoStoreWrapper.

While the wrapper shouldn't be the final destination for this cache it does help with our current multi-process support which relied on the CryptoStore::clear_caches() method. This method is now gone and no further caches should be added to the store implementations.

Review commit by commit isn't advised as the API changed and changed back when the cache was removed and re-added.

  • Public API changes documented in changelogs (optional)

@poljar poljar marked this pull request as ready for review August 6, 2024 11:59
@poljar poljar requested a review from a team as a code owner August 6, 2024 11:59
@poljar poljar requested review from jmartinesp and removed request for a team August 6, 2024 11:59
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

Attention: Patch coverage is 98.71795% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.07%. Comparing base (c83fa3a) to head (7a78443).

Files Patch % Lines
...atrix-sdk-crypto/src/store/crypto_store_wrapper.rs 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3799      +/-   ##
==========================================
- Coverage   84.11%   84.07%   -0.05%     
==========================================
  Files         263      263              
  Lines       27476    27475       -1     
==========================================
- Hits        23111    23099      -12     
- Misses       4365     4376      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

Having no clue about the underlying crypto logic the code makes sense to me. LGTM but maybe we want someone else's review here too.

Copy link
Contributor

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Looks good.

@poljar poljar force-pushed the poljar/session-cache-removal branch from d4483e9 to 7a78443 Compare August 6, 2024 12:54
@poljar poljar enabled auto-merge (rebase) August 6, 2024 12:54
@poljar poljar merged commit c3848ca into main Aug 6, 2024
40 checks passed
@poljar poljar deleted the poljar/session-cache-removal branch August 6, 2024 13:10
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