Bug #1870 UI component

When we spin retrying a rename, show a UI notification. Rather raw but much better than silently doing nothing for an extended period.
rust-lang · Jun 2, 2019 · 20ff7fd · 20ff7fd
1 parent b3325f9
commit 20ff7fd
Show file tree

Hide file tree

Showing 6 changed files with 93 additions and 28 deletions.
diff --git a/src/dist/component/components.rs b/src/dist/component/components.rs
@@ -194,7 +194,7 @@ impl Component {
         let temp = tx.temp().new_file()?;
         utils::filter_file("components", &abs_path, &temp, |l| (l != self.name))?;
         tx.modify_file(path)?;
-        utils::rename_file("components", &temp, &abs_path)?;
+        utils::rename_file("components", &temp, &abs_path, tx.notify_handler())?;
 
         // TODO: If this is the last component remove the components file
         // and the version file.

diff --git a/src/dist/component/transaction.rs b/src/dist/component/transaction.rs
@@ -93,7 +93,13 @@ impl<'a> Transaction<'a> {
     /// Remove a file from a relative path to the install prefix.
     pub fn remove_file(&mut self, component: &str, relpath: PathBuf) -> Result<()> {
         assert!(relpath.is_relative());
-        let item = ChangedItem::remove_file(&self.prefix, component, relpath, &self.temp_cfg)?;
+        let item = ChangedItem::remove_file(
+            &self.prefix,
+            component,
+            relpath,
+            &self.temp_cfg,
+            self.notify_handler(),
+        )?;
         self.change(item);
         Ok(())
     }
@@ -102,7 +108,13 @@ impl<'a> Transaction<'a> {
     /// install prefix.
     pub fn remove_dir(&mut self, component: &str, relpath: PathBuf) -> Result<()> {
         assert!(relpath.is_relative());
-        let item = ChangedItem::remove_dir(&self.prefix, component, relpath, &self.temp_cfg)?;
+        let item = ChangedItem::remove_dir(
+            &self.prefix,
+            component,
+            relpath,
+            &self.temp_cfg,
+            self.notify_handler(),
+        )?;
         self.change(item);
         Ok(())
     }
@@ -136,15 +148,17 @@ impl<'a> Transaction<'a> {
     /// Move a file to a relative path of the install prefix.
     pub fn move_file(&mut self, component: &str, relpath: PathBuf, src: &Path) -> Result<()> {
         assert!(relpath.is_relative());
-        let item = ChangedItem::move_file(&self.prefix, component, relpath, src)?;
+        let item =
+            ChangedItem::move_file(&self.prefix, component, relpath, src, self.notify_handler())?;
         self.change(item);
         Ok(())
     }
 
     /// Recursively move a directory to a relative path of the install prefix.
     pub fn move_dir(&mut self, component: &str, relpath: PathBuf, src: &Path) -> Result<()> {
         assert!(relpath.is_relative());
-        let item = ChangedItem::move_dir(&self.prefix, component, relpath, src)?;
+        let item =
+            ChangedItem::move_dir(&self.prefix, component, relpath, src, self.notify_handler())?;
         self.change(item);
         Ok(())
     }
@@ -166,7 +180,7 @@ impl<'a> Drop for Transaction<'a> {
             for item in self.changes.iter().rev() {
                 // ok_ntfy!(self.notify_handler,
                 //          Notification::NonFatalError,
-                match item.roll_back(&self.prefix) {
+                match item.roll_back(&self.prefix, self.notify_handler()) {
                     Ok(()) => {}
                     Err(e) => {
                         (self.notify_handler)(Notification::NonFatalError(&e));
@@ -191,20 +205,20 @@ enum ChangedItem<'a> {
 }
 
 impl<'a> ChangedItem<'a> {
-    fn roll_back(&self, prefix: &InstallPrefix) -> Result<()> {
+    fn roll_back(
+        &self,
+        prefix: &InstallPrefix,
+        notify: &'a dyn Fn(Notification<'_>),
+    ) -> Result<()> {
         use self::ChangedItem::*;
         match self {
             AddedFile(path) => utils::remove_file("component", &prefix.abs_path(path))?,
-            AddedDir(path) => {
-                utils::remove_dir("component", &prefix.abs_path(path), &|_: Notification<
-                    '_,
-                >| ())?
-            }
+            AddedDir(path) => utils::remove_dir("component", &prefix.abs_path(path), notify)?,
             RemovedFile(path, tmp) | ModifiedFile(path, Some(tmp)) => {
-                utils::rename_file("component", &tmp, &prefix.abs_path(path))?
+                utils::rename_file("component", &tmp, &prefix.abs_path(path), notify)?
             }
             RemovedDir(path, tmp) => {
-                utils::rename_dir("component", &tmp.join("bk"), &prefix.abs_path(path))?
+                utils::rename_dir("component", &tmp.join("bk"), &prefix.abs_path(path), notify)?
             }
             ModifiedFile(path, None) => {
                 let abs_path = prefix.abs_path(path);
@@ -265,6 +279,7 @@ impl<'a> ChangedItem<'a> {
         component: &str,
         relpath: PathBuf,
         temp_cfg: &'a temp::Cfg,
+        notify: &'a dyn Fn(Notification<'_>),
     ) -> Result<Self> {
         let abs_path = prefix.abs_path(&relpath);
         let backup = temp_cfg.new_file()?;
@@ -275,7 +290,7 @@ impl<'a> ChangedItem<'a> {
             }
             .into())
         } else {
-            utils::rename_file("component", &abs_path, &backup)?;
+            utils::rename_file("component", &abs_path, &backup, notify)?;
             Ok(ChangedItem::RemovedFile(relpath, backup))
         }
     }
@@ -284,6 +299,7 @@ impl<'a> ChangedItem<'a> {
         component: &str,
         relpath: PathBuf,
         temp_cfg: &'a temp::Cfg,
+        notify: &'a dyn Fn(Notification<'_>),
     ) -> Result<Self> {
         let abs_path = prefix.abs_path(&relpath);
         let backup = temp_cfg.new_directory()?;
@@ -294,7 +310,7 @@ impl<'a> ChangedItem<'a> {
             }
             .into())
         } else {
-            utils::rename_dir("component", &abs_path, &backup.join("bk"))?;
+            utils::rename_dir("component", &abs_path, &backup.join("bk"), notify)?;
             Ok(ChangedItem::RemovedDir(relpath, backup))
         }
     }
@@ -321,19 +337,21 @@ impl<'a> ChangedItem<'a> {
         component: &str,
         relpath: PathBuf,
         src: &Path,
+        notify: &'a dyn Fn(Notification<'_>),
     ) -> Result<Self> {
         let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
-        utils::rename_file("component", src, &abs_path)?;
+        utils::rename_file("component", src, &abs_path, notify)?;
         Ok(ChangedItem::AddedFile(relpath))
     }
     fn move_dir(
         prefix: &InstallPrefix,
         component: &str,
         relpath: PathBuf,
         src: &Path,
+        notify: &'a dyn Fn(Notification<'_>),
     ) -> Result<Self> {
         let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
-        utils::rename_dir("component", src, &abs_path)?;
+        utils::rename_dir("component", src, &abs_path, notify)?;
         Ok(ChangedItem::AddedDir(relpath))
     }
 }
diff --git a/src/dist/download.rs b/src/dist/download.rs
@@ -88,7 +88,12 @@ impl<'a> DownloadCfg<'a> {
         } else {
             (self.notify_handler)(Notification::ChecksumValid(&url.to_string()));
 
-            utils::rename_file("downloaded", &partial_file_path, &target_file)?;
+            utils::rename_file(
+                "downloaded",
+                &partial_file_path,
+                &target_file,
+                self.notify_handler,
+            )?;
             Ok(File { path: target_file })
         }
     }

diff --git a/src/utils/notifications.rs b/src/utils/notifications.rs
@@ -28,14 +28,21 @@ pub enum Notification<'a> {
     ResumingPartialDownload,
     UsingCurl,
     UsingReqwest,
+    /// Renaming encountered a file in use error and is retrying.
+    /// The InUse aspect is a heuristic - the OS specifies
+    /// Permission denied, but as we work in users home dirs and
+    /// running programs like virus scanner are known to cause this
+    /// the heuristic is quite good.
+    RenameInUse(&'a Path, &'a Path),
 }
 
 impl<'a> Notification<'a> {
     pub fn level(&self) -> NotificationLevel {
         use self::Notification::*;
         match self {
-            CreatingDirectory(_, _) | RemovingDirectory(_, _) => NotificationLevel::Verbose,
-            LinkingDirectory(_, _)
+            CreatingDirectory(_, _)
+            | RemovingDirectory(_, _)
+            | LinkingDirectory(_, _)
             | CopyingDirectory(_, _)
             | DownloadingFile(_, _)
             | DownloadContentLengthReceived(_)
@@ -46,6 +53,7 @@ impl<'a> Notification<'a> {
             | ResumingPartialDownload
             | UsingCurl
             | UsingReqwest => NotificationLevel::Verbose,
+            RenameInUse(_, _) => NotificationLevel::Info,
             NoCanonicalPath(_) => NotificationLevel::Warn,
         }
     }
@@ -63,6 +71,12 @@ impl<'a> Display for Notification<'a> {
             RemovingDirectory(name, path) => {
                 write!(f, "removing {} directory: '{}'", name, path.display())
             }
+            RenameInUse(src, dest) => write!(
+                f,
+                "retrying renaming '{}' to '{}'",
+                src.display(),
+                dest.display()
+            ),
             DownloadingFile(url, _) => write!(f, "downloading file from: '{}'", url),
             DownloadContentLengthReceived(len) => write!(f, "download size is: '{}'", len),
             DownloadDataReceived(data) => write!(f, "received some data of size {}", data.len()),

diff --git a/src/utils/utils.rs b/src/utils/utils.rs
@@ -75,12 +75,28 @@ pub fn write_str(name: &'static str, file: &mut File, path: &Path, s: &str) -> R
     })
 }
 
-pub fn rename_file(name: &'static str, src: &Path, dest: &Path) -> Result<()> {
-    rename(name, src, dest)
+pub fn rename_file<'a, N>(
+    name: &'static str,
+    src: &'a Path,
+    dest: &'a Path,
+    notify: &'a dyn Fn(N),
+) -> Result<()>
+where
+    N: From<Notification<'a>>,
+{
+    rename(name, src, dest, notify)
 }
 
-pub fn rename_dir(name: &'static str, src: &Path, dest: &Path) -> Result<()> {
-    rename(name, src, dest)
+pub fn rename_dir<'a, N>(
+    name: &'static str,
+    src: &'a Path,
+    dest: &'a Path,
+    notify: &'a dyn Fn(N),
+) -> Result<()>
+where
+    N: From<Notification<'a>>,
+{
+    rename(name, src, dest, notify)
 }
 
 pub fn filter_file<F: FnMut(&str) -> bool>(
@@ -664,7 +680,15 @@ pub fn toolchain_sort<T: AsRef<str>>(v: &mut Vec<T>) {
     });
 }
 
-fn rename(name: &'static str, src: &Path, dest: &Path) -> Result<()> {
+fn rename<'a, N>(
+    name: &'static str,
+    src: &'a Path,
+    dest: &'a Path,
+    notify_handler: &'a dyn Fn(N),
+) -> Result<()>
+where
+    N: From<Notification<'a>>,
+{
     // https://github.com/rust-lang/rustup.rs/issues/1870
     // 21 fib steps from 1 sums to ~28 seconds, hopefully more than enough
     // for our previous poor performance that avoided the race condition with
@@ -674,7 +698,10 @@ fn rename(name: &'static str, src: &Path, dest: &Path) -> Result<()> {
         || match fs::rename(src, dest) {
             Ok(v) => OperationResult::Ok(v),
             Err(e) => match e.kind() {
-                io::ErrorKind::PermissionDenied => OperationResult::Retry(e),
+                io::ErrorKind::PermissionDenied => {
+                    notify_handler(Notification::RenameInUse(&src, &dest).into());
+                    OperationResult::Retry(e)
+                }
                 _ => OperationResult::Err(e),
             },
         },

diff --git a/tests/cli-self-upd.rs b/tests/cli-self-upd.rs
@@ -11,6 +11,7 @@ use crate::mock::{get_path, restore_path};
 use lazy_static::lazy_static;
 use remove_dir_all::remove_dir_all;
 use rustup::utils::{raw, utils};
+use rustup::Notification;
 use std::env;
 use std::env::consts::EXE_SUFFIX;
 use std::fs;
@@ -952,7 +953,7 @@ fn rustup_init_works_with_weird_names() {
     setup(&|config| {
         let old = config.exedir.join(&format!("rustup-init{}", EXE_SUFFIX));
         let new = config.exedir.join(&format!("rustup-init(2){}", EXE_SUFFIX));
-        utils::rename_file("test", &old, &new).unwrap();
+        utils::rename_file("test", &old, &new, &|_:Notification<'_>|{}).unwrap();
         expect_ok(config, &["rustup-init(2)", "-y"]);
         let rustup = config.cargodir.join(&format!("bin/rustup{}", EXE_SUFFIX));
         assert!(rustup.exists());