Skip to content

Commit c0f6fbc

Browse files
authored
Transaction retry: Remove unnecessary Arc and callback boxing (#6922)
We do still need the `Mutex` around `self.inner` here, even though there is never any contention. To remove the mutex, we'd need `retry_callback()` to take `&mut self`, not `&self`, so that it could directly mutate `self.inner`. However, `transaction_async_with_retry` (defined in `async-bb8-diesel`) has this requirement on the retry closure: ```rust RetryFut: FutureExt<Output = bool> + Send, RetryFunc: Fn() -> RetryFut + Send + Sync, ``` We'd need to change this `Fn` to `FnMut`. That's doable, but then the closure we attempt to construct (`|| self.retry_callback()`) wouldn't pass the borrow checker: the future returned by `self.retry_callback()` would hold `&mut self`, but the generic bounds above would allow `transaction_async_with_retry` to call the closure multiple times and get multiple `RetryFut`s, therefore constructing multiple futures that all hold `&mut self`s (which is obviously not okay). `transaction_async_with_retry` doesn't actually _do_ that - it only constructs one at a time and always awaits it before creating another one - but I don't think there's any way to express that in the type system. We could fix this by moving `self` into the closure, but that defeats the point: we need to access the details recorded in `self.inner` _after_ `transaction_async_with_retry`, so we can't move `self` (or even `self.inner`) into the closure. All this to say: I think the mutex this has is the most straightforward way to make this all okay, so this PR only changes some of the somewhat unnecessary surrounding code.
1 parent dceed97 commit c0f6fbc

File tree

1 file changed

+3
-15
lines changed

1 file changed

+3
-15
lines changed

nexus/db-queries/src/transaction_retry.rs

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -106,15 +106,14 @@ impl RetryHelper {
106106
+ Send
107107
+ Sync,
108108
{
109-
let slef = Arc::new(self);
110109
let result = conn
111-
.transaction_async_with_retry(f, slef.clone().as_callback())
110+
.transaction_async_with_retry(f, || self.retry_callback())
112111
.await;
113112

114-
let retry_info = slef.inner.lock().unwrap();
113+
let retry_info = self.inner.lock().unwrap();
115114
if retry_info.has_retried() {
116115
info!(
117-
slef.log,
116+
self.log,
118117
"transaction completed";
119118
"attempts" => retry_info.attempts,
120119
);
@@ -169,17 +168,6 @@ impl RetryHelper {
169168
let inner = self.inner.lock().unwrap().tick();
170169
return inner.attempts < MAX_RETRY_ATTEMPTS;
171170
}
172-
173-
// Converts this function to a retryable callback that can be used from
174-
// "transaction_async_with_retry".
175-
fn as_callback(
176-
self: Arc<Self>,
177-
) -> impl Fn() -> futures::future::BoxFuture<'static, bool> {
178-
move || {
179-
let r = self.clone();
180-
Box::pin(async move { r.retry_callback().await })
181-
}
182-
}
183171
}
184172

185173
impl oximeter::Producer for Producer {

0 commit comments

Comments
 (0)