deduplicating handler: handle cancellation by repeating the query, if needs be #3533
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3533 +/- ##
==========================================
- Coverage 83.79% 83.78% -0.01%
==========================================
Files 254 254
Lines 25734 25744 +10
==========================================
+ Hits 21563 21570 +7
- Misses 4171 4174 +3 ☔ View full report in Codecov by Sentry. |
bnjbvr
force-pushed
the
bnjbvr/deduplicating-handler-re-run-query
branch
from
ee3fbef to
f958fb7
Compare
| /// The query hasn't completed yet. This doesn't mean it hasn't *started* | ||
| /// yet, but rather that it couldn't get to completion: some | ||
| /// intermediate steps might have run. | ||
| NotFinishedYet, |
There was a problem hiding this comment.
NotFinishedYet sounds like we should wait and see if it'll get finished next time, if we're sure that this only happens only if we cancel the future, perhaps Cancelled would be a better name.
| // Assume a successful request; we'll modify the result in case of failures | ||
| // later. | ||
| let request_mutex = Arc::new(Mutex::new(Ok(()))); | ||
| // Start at the `None` state to indicate we haven't completed the request yet. |
There was a problem hiding this comment.
That's not the None state 😅. I see now why you picked NotFinishedYet, though it still seems that we could say something like:
Let's assume the cancelled state, if we succeed or fail we'll modify the result.
There was a problem hiding this comment.
Haha, remnant from a previous push. Thanks, I've used Cancelled in lieu of NotFinishedYet, and tweaked the comment accordingly.
(Also added a note about the code future which must be idempotent if run multiple times.)
… a few comments around the code future
Solves one potential issue spotted in #3529. If the following sequence happened:
Then, in the previous version of the code, request B would finish and assume that A successfully completed, while this is not the case.
This new version handles that by repeating the deduplicated query. This assumes that it's fine to repeat the entire query, which is not a light assumption, but looking at the uses of the deduplicating handler (/members, sending a read receipt for a room, reading the encryption state for a room), this should be fine.