From a2db48d6498922c0ad146e7fc95c3e861555e857 Mon Sep 17 00:00:00 2001 From: Aria Desires Date: Mon, 27 Jan 2025 18:51:36 -0500 Subject: [PATCH] fix async windows file persist retries (#11008) The previous two versions of the code were bugged and would always produce None when you retried (producing a hard LostState error). --- crates/uv-fs/src/lib.rs | 47 ++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/crates/uv-fs/src/lib.rs b/crates/uv-fs/src/lib.rs index 8f6cbc937..940af094d 100644 --- a/crates/uv-fs/src/lib.rs +++ b/crates/uv-fs/src/lib.rs @@ -310,22 +310,45 @@ pub async fn persist_with_retry( // See: & let to = to.as_ref(); - // the `NamedTempFile` `persist` method consumes `self`, and returns it back inside the Error in case of `PersistError` + // Ok there's a lot of complex ownership stuff going on here. + // + // the `NamedTempFile` `persist` method consumes `self`, and returns it back inside + // the Error in case of `PersistError`: // https://docs.rs/tempfile/latest/tempfile/struct.NamedTempFile.html#method.persist - // So we will update the `from` optional value in safe and borrow-checker friendly way every retry - // Allows us to use the NamedTempFile inside a FnMut closure used for backoff::retry - let mut from = Some(from); - let persist = move || { - // Needed because we cannot move out of `from`, a captured variable in an `FnMut` closure, and then pass it to the async move block - let mut from: Option = from.take(); + // So every time we fail, we need to reset the `NamedTempFile` to try again. + // + // Every time we (re)try we call this outer closure (`let persist = ...`), so it needs to + // be at least a `FnMut` (as opposed to `Fnonce`). However the closure needs to return a + // totally owned `Future` (so effectively it returns a `FnOnce`). + // + // But if the `Future` is totally owned it *necessarily* can't write back the `NamedTempFile` + // to somewhere the outer `FnMut` can see using references. So we need to use `Arc`s + // with interior mutability (`Mutex`) to have the closure and all the Futures it creates share + // a single memory location that the `NamedTempFile` can be shuttled in and out of. + // + // In spite of the Mutex all of this code will run logically serially, so there shouldn't be a + // chance for a race where we try to get the `NamedTempFile` but it's actually None. The code + // is just written pedantically/robustly. + let from = std::sync::Arc::new(std::sync::Mutex::new(Some(from))); + let persist = || { + // Turn our by-ref-captured Arc into an owned Arc that the Future can capture by-value + let from2 = from.clone(); async move { - if let Some(file) = from.take() { + let maybe_file: Option = from2 + .lock() + .map_err(|_| PersistRetryError::LostState)? + .take(); + if let Some(file) = maybe_file { file.persist(to).map_err(|err| { - let error_message = err.to_string(); - // Set back the NamedTempFile returned back by the Error - from = Some(err.file); - PersistRetryError::Persist(error_message) + let error_message: String = err.to_string(); + // Set back the `NamedTempFile` returned back by the Error + if let Ok(mut guard) = from2.lock() { + *guard = Some(err.file); + PersistRetryError::Persist(error_message) + } else { + PersistRetryError::LostState + } }) } else { Err(PersistRetryError::LostState)