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

task(send queue): demote some assertions back to logged errors #4269

Merged
merged 1 commit into from
Nov 15, 2024
Merged

task(send queue): demote some assertions back to logged errors #4269

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions crates/matrix-sdk/src/send_queue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,13 @@ impl QueueStorage {
transaction_id: request.transaction_id.clone(),
cancel_upload: cancel_upload_tx,
});
assert!(prev.is_none());

if let Some(prev) = prev {
error!(
prev_txn = ?prev.transaction_id,
"a previous request was still active while picking a new one"
);
}

Ok(Some((request.clone(), cancel_upload_rx)))
} else {
Expand All @@ -971,10 +977,11 @@ impl QueueStorage {
/// be removed from the queue later.
async fn mark_as_not_being_sent(&self, transaction_id: &TransactionId) {
let was_being_sent = self.being_sent.write().await.take();
assert_eq!(
was_being_sent.as_ref().map(|info| info.transaction_id.as_ref()),
Some(transaction_id)
);

let prev_txn = was_being_sent.as_ref().map(|info| info.transaction_id.as_ref());
if prev_txn != Some(transaction_id) {
error!(prev_txn = ?prev_txn, "previous active request didn't match that we expect (after transient error)");
}
}

/// Marks a request popped with [`Self::peek_next_to_send`] and identified
Expand All @@ -988,10 +995,11 @@ impl QueueStorage {
// Keep the lock until we're done touching the storage.
let mut being_sent = self.being_sent.write().await;
let was_being_sent = being_sent.take();
assert_eq!(
was_being_sent.as_ref().map(|info| info.transaction_id.as_ref()),
Some(transaction_id)
);

let prev_txn = was_being_sent.as_ref().map(|info| info.transaction_id.as_ref());
if prev_txn != Some(transaction_id) {
error!(prev_txn = ?prev_txn, "previous active request didn't match that we expect (after permanent error)");
}

Ok(self
.client()?
Expand Down Expand Up @@ -1023,10 +1031,11 @@ impl QueueStorage {
// Keep the lock until we're done touching the storage.
let mut being_sent = self.being_sent.write().await;
let was_being_sent = being_sent.take();
assert_eq!(
was_being_sent.as_ref().map(|info| info.transaction_id.as_ref()),
Some(transaction_id)
);

let prev_txn = was_being_sent.as_ref().map(|info| info.transaction_id.as_ref());
if prev_txn != Some(transaction_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The assert would have aborted execution here, while we now continue on, is that fine? Don't we want to perhaps return early?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would be a sign that the system is in a weird state, but the transaction_id passed as a parameter is the important value that is correct above all, so the current code is fine.

error!(prev_txn = ?prev_txn, "previous active request didn't match that we expect (after successful send");
}

let client = self.client()?;
let store = client.store();
Expand Down
Loading