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

Retry decryption from a long-lived async task, instead of lots of detached ones #4715

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

Conversation

andybalaam
Copy link
Member

@andybalaam andybalaam commented Feb 25, 2025

Part of element-hq/element-meta#2697

In response to a comment on #4644 - avoid spawning a detached async task every time we have some things to decrypt because some Megolm sessions updated.

I strongly suggest reviewing commit by commit, since this is structured as a set of refactorings that hopefully make sense individually.

Instead, we create one async task and hold it inside a new struct called RetryDecryptionTask. This struct holds one end of channel, and sends retry requests to the async task, which stops when the channel is closed, which will happen when the RetryDecryptionTask struct is dropped.

So we keep the feature that retrying decryption does not block the main processing, but we lose the spawning of lots of async tasks that are not kept track of.

As written, the task is not cancellable, but this could be added if we think we need it.

Copy link

codecov bot commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 73.91304% with 24 lines in your changes missing coverage. Please review.

Project coverage is 85.93%. Comparing base (2f3cab4) to head (9952bc3).
Report is 32 commits behind head on main.

Files with missing lines Patch % Lines
...i/src/timeline/controller/decryption_retry_task.rs 72.94% 23 Missing ⚠️
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4715      +/-   ##
==========================================
- Coverage   85.94%   85.93%   -0.02%     
==========================================
  Files         290      291       +1     
  Lines       33839    33856      +17     
==========================================
+ Hits        29084    29093       +9     
- Misses       4755     4763       +8     

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

@andybalaam
Copy link
Member Author

This lines that codecov is complaining about missing coverage are pretty much all lifted-and-shifted from the their old location into decryption_retry_task.rs. This code is tested by existing tests in crates/matrix-sdk-ui/src/timeline/tests/encryption.rs and crates/matrix-sdk-ui/src/timeline/tests/read_receipts.rs. Whilst it would be nice to add unit tests closer to the code, these cover the more real-world cases, and this PR is a detour-on-a-detour for me :-)

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.

1 participant