feat(send queue): allow aborting a media upload #4262
Conversation
crates/matrix-sdk/src/send_queue.rs
Outdated
| /// All the queued requests that are being sent at the moment. | ||
| /// | ||
| /// It also serves as an internal lock on the storage backend. | ||
| being_sent: Arc<RwLock<BTreeSet<OwnedTransactionId>>>, | ||
| being_sent: Arc<RwLock<Option<OwnedTransactionId>>>, |
There was a problem hiding this comment.
Maybe the doc comment doesn't match anymore as there is only 1 request at a time?
crates/matrix-sdk/src/send_queue.rs
Outdated
| transaction_id: OwnedTransactionId, | ||
|
|
||
| /// For an upload request, a trigger to cancel the upload before it | ||
| /// completed. |
There was a problem hiding this comment.
Minor typo maybe? Either 'it's completed' or 'it completes'.
| if let Some(info) = being_sent.as_ref() { | ||
| if info.transaction_id == *thumbnail_txn { | ||
| // SAFETY: we knew it was Some(), two lines above. | ||
| let info = being_sent.take().unwrap(); |
There was a problem hiding this comment.
Rookie Rust dev question: why do we need another info here?
There was a problem hiding this comment.
Good question! Above it was taken as_ref(), so a &BeingSentInfo; the cancellation requires triggering the oneshot::channel, which consumes it by ownership, so we need to take BeingSentInfo by ownership here; at this point it's fine because we know it's the last use. I'll add a comment and maybe a tiny function helper to make this clearer.
There was a problem hiding this comment.
Ah, I see, I didn't realise the channel is consumed 👌 .
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4262 +/- ##
==========================================
+ Coverage 84.92% 84.95% +0.02%
==========================================
Files 274 274
Lines 29858 29939 +81
==========================================
+ Hits 25358 25434 +76
- Misses 4500 4505 +5 ☔ View full report in Codecov by Sentry. |
There can be at most one thing being sent by the send queue, so make this super explicit.
…nail upload is active
bnjbvr
force-pushed
the
bnjbvr/send-queue-abort
branch
from
de6064c
to
106308c
Compare
This is a bit tricky, because we need to care about all possible states of the sending state machine, and do something different in each case:
Fortunately, it's pretty easy to test all the cases, so I've done just that. Many LOCs are just tests, so don't be too afraid by it.
Part of #4201.