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

[andybalaam]

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.

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.

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

[codecov]

Codecov Report

Attention: Patch coverage is 73.40426% with 25 lines in your changes missing coverage. Please review.

Project coverage is 86.14%. Comparing base (7a0bf9b) to head (9cb0630).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
...i/src/timeline/controller/decryption_retry_task.rs	72.41%	24 Missing ⚠️
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4715   +/-   ##
=======================================
  Coverage   86.14%   86.14%           
=======================================
  Files         291      292    +1     
  Lines       34308    34327   +19     
=======================================
+ Hits        29553    29570   +17     
- Misses       4755     4757    +2     

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

[andybalaam]

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 :-)

[andybalaam]

@poljar is this something like what you intended in https://github.com/matrix-org/matrix-rust-sdk/pull/4644/files#r1963599148 ?

[poljar]

Hmm, we're still dropping the JoinHandle though the task will get killed because the sender gets dropped, which in turn means the receiver will get a None.

That's a bit implicit compared to holding on to the JoinHandle and cancelling in the drop() call of `DecryptionRetryTask, but I guess there's nothing wrong with it.

[andybalaam]

Yeah, I tried to join in drop() but it's async, so I can't do it.

[poljar]

Oh, you wouldn't await the join in the drop() call, you would just abort() the task. But perhaps it's cleaner to let the task naturally die due to the Sender being gone.

[bnjbvr]

I think keeping the join handle and manually aborting shows a clear ownership of the task, and would be a tiny bit clearer, even in terms of discoverability: we'd see the JoinHandle in the struct, thus know that this is a task's owner.

[poljar]

At first I was thinking this as well. But then the task could stop in two ways:

1. It ends because the receiver receives a None.
2. It gets cancelled.

And then I would need to think about cancel safety of all the things the task contains. Which made me conclude that it's fine to let the task die of natural causes the way it's done.

[bnjbvr]

Fair; my argument in terms of discoverability still holds IMO, can we store the task in the struct at least?

[poljar]

Sure, that's probably a wise idea.

[andybalaam]

Done in f39a969

[poljar]

@poljar is this something like what you intended in https://github.com/matrix-org/matrix-rust-sdk/pull/4644/files#r1963599148 ?

Yeah, I think that's fine, I explained things a bit more here: #4715 (review)

[poljar]

Nice work, thanks for splitting this into many small commits, made the review easier. I left one nit, but I'll approve this in advance.

There's also a typo in one commit:

Adjust tsts that retry decryption to wait with

In any case, this should make it much easier to move the redecryption logic into the correct place, wherever we decide this to be.

[poljar]

Sorry to change my mind, could you please put the JoinHandle into the DecryptionRetryTask.

I shouldn't be used for anything, just to let people know that the task is owned by this struct.

[poljar]

Thanks for the last minute change, we can merge this after you make the compiler happy with the right types.