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.

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

Jump to bottom

FFI Bindings: Expose CryptoStore clear caches #3462

Merged
merged 5 commits into from
May 28, 2024
Merged

FFI Bindings: Expose CryptoStore clear caches #3462

merged 5 commits into from
May 28, 2024

Conversation

BillCarsonFr
Copy link
Member

Exposes the crypto store clear_caches method on the OlmMachine to be used by EI to invalidate cache when multiprocess is involed. (see EXI equivalent #3338)

  • Public API changes documented in changelogs (optional)

Signed-off-by:

This was referenced May 24, 2024
Merged
Open
@BillCarsonFr BillCarsonFr marked this pull request as ready for review May 27, 2024 08:03
@BillCarsonFr BillCarsonFr requested a review from a team as a code owner May 27, 2024 08:03
@BillCarsonFr BillCarsonFr requested review from Hywan and removed request for a team May 27, 2024 08:03
@codecov Codecov
Copy link

codecov bot commented May 27, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 83.29%. Comparing base (1dc3709) to head (0e456cd).
Report is 19 commits behind head on main.

Files Patch % Lines
crates/matrix-sdk-crypto/src/machine.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3462   +/-   ##
=======================================
  Coverage   83.29%   83.29%           
=======================================
  Files         248      248           
  Lines       25236    25237    +1     
=======================================
+ Hits        21021    21022    +1     
  Misses       4215     4215

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

Hywan
Hywan requested changes May 27, 2024
View reviewed changes
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

I'm a bit reluctant to approve this PR because abusing the clear_crypto_cache can slow things down. It's kind of obvious though. Nonetheless, it is still safe to clear the crypto cache too often.

I'll approve it once the feedback is addressed.

bindings/matrix-sdk-crypto-ffi/src/machine.rs Outdated Show resolved Hide resolved
@BillCarsonFr @Hywan
FFI Bindings: Review, remove runtime block_on, use await
a042b3d 
Co-authored-by: Ivan Enderlin <ivan@mnt.io>
Signed-off-by: Valere <bill.carson@valrsoft.com>
@BillCarsonFr
Copy link
Member Author

I'm a bit reluctant to approve this PR because abusing the clear_crypto_cache can slow things down. It's kind of obvious though. Nonetheless, it is still safe to clear the crypto cache too often.

I'll approve it once the feedback is addressed.

Yes more details here element-hq/element-ios#7790

@BillCarsonFr BillCarsonFr requested a review from Hywan May 27, 2024 10:55
poljar
poljar reviewed May 28, 2024
View reviewed changes
bindings/matrix-sdk-crypto-ffi/src/machine.rs
/// When used in a multi-process context this cache will get outdated.
/// If the machine is used by another process, the cache must be
/// invalidating when the main process is resumed.
pub async fn clear_crypto_cache(&self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test this? It doesn't crash? Don't you need to modify the export line at the top of the impl block to #[uniffi::export(async_runtime = "tokio")] for it not to crash?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I have tried, and no crash

poljar
poljar approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@poljar poljar 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.

@Hywan Hywan merged commit eddaf96 into main May 28, 2024
38 checks passed
@Hywan Hywan deleted the valere/binding_ffi_crypto_expose_clear_caches branch May 28, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hywan Hywan Hywan approved these changes

@poljar poljar poljar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BillCarsonFr @poljar @Hywan