Skip to content

Commit e41ca88

Browse files
djcdjc
committed
notifications: log directly for retrying renames
1 parent 94859c7 commit e41ca88

File tree

6 files changed

+27
-86
lines changed

6 files changed

+27
-86
lines changed

‎src/dist/component/components.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,7 +266,7 @@ impl Component {
266266
let temp = tx.temp().new_file()?;
267267
utils::filter_file("components", &abs_path, &temp, |l| l != self.name)?;
268268
tx.modify_file(path)?;
269-
utils::rename("components", &temp, &abs_path, tx.notify_handler(), process)?;
269+
utils::rename("components", &temp, &abs_path, process)?;
270270

271271
// TODO: If this is the last component remove the components file
272272
// and the version file.

‎src/dist/component/transaction.rs‎

Lines changed: 13 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -99,14 +99,8 @@ impl<'a> Transaction<'a> {
9999
/// Remove a file from a relative path to the install prefix.
100100
pub fn remove_file(&mut self, component: &str, relpath: PathBuf) -> Result<()> {
101101
assert!(relpath.is_relative());
102-
let item = ChangedItem::remove_file(
103-
&self.prefix,
104-
component,
105-
relpath,
106-
self.tmp_cx,
107-
self.notify_handler(),
108-
self.process,
109-
)?;
102+
let item =
103+
ChangedItem::remove_file(&self.prefix, component, relpath, self.tmp_cx, self.process)?;
110104
self.change(item);
111105
Ok(())
112106
}
@@ -115,14 +109,8 @@ impl<'a> Transaction<'a> {
115109
/// install prefix.
116110
pub fn remove_dir(&mut self, component: &str, relpath: PathBuf) -> Result<()> {
117111
assert!(relpath.is_relative());
118-
let item = ChangedItem::remove_dir(
119-
&self.prefix,
120-
component,
121-
relpath,
122-
self.tmp_cx,
123-
self.notify_handler(),
124-
self.process,
125-
)?;
112+
let item =
113+
ChangedItem::remove_dir(&self.prefix, component, relpath, self.tmp_cx, self.process)?;
126114
self.change(item);
127115
Ok(())
128116
}
@@ -161,39 +149,22 @@ impl<'a> Transaction<'a> {
161149
src: &Path,
162150
) -> Result<()> {
163151
assert!(relpath.is_relative());
164-
let item = ChangedItem::move_file(
165-
&self.prefix,
166-
component,
167-
relpath,
168-
src,
169-
self.notify_handler(),
170-
self.process,
171-
)?;
152+
let item = ChangedItem::move_file(&self.prefix, component, relpath, src, self.process)?;
172153
self.change(item);
173154
Ok(())
174155
}
175156

176157
/// Recursively move a directory to a relative path of the install prefix.
177158
pub(crate) fn move_dir(&mut self, component: &str, relpath: PathBuf, src: &Path) -> Result<()> {
178159
assert!(relpath.is_relative());
179-
let item = ChangedItem::move_dir(
180-
&self.prefix,
181-
component,
182-
relpath,
183-
src,
184-
self.notify_handler(),
185-
self.process,
186-
)?;
160+
let item = ChangedItem::move_dir(&self.prefix, component, relpath, src, self.process)?;
187161
self.change(item);
188162
Ok(())
189163
}
190164

191165
pub(crate) fn temp(&self) -> &'a temp::Context {
192166
self.tmp_cx
193167
}
194-
pub(crate) fn notify_handler(&self) -> &'a dyn Fn(Notification<'_>) {
195-
self.notify_handler
196-
}
197168
}
198169

199170
/// If a Transaction is dropped without being committed, the changes
@@ -205,7 +176,7 @@ impl Drop for Transaction<'_> {
205176
for item in self.changes.iter().rev() {
206177
// ok_ntfy!(self.notify_handler,
207178
// Notification::NonFatalError,
208-
match item.roll_back(&self.prefix, self.notify_handler(), self.process) {
179+
match item.roll_back(&self.prefix, self.process) {
209180
Ok(()) => {}
210181
Err(e) => {
211182
(self.notify_handler)(Notification::NonFatalError(&e));
@@ -230,24 +201,18 @@ enum ChangedItem<'a> {
230201
}
231202

232203
impl<'a> ChangedItem<'a> {
233-
fn roll_back(
234-
&self,
235-
prefix: &InstallPrefix,
236-
notify: &'a dyn Fn(Notification<'_>),
237-
process: &Process,
238-
) -> Result<()> {
204+
fn roll_back(&self, prefix: &InstallPrefix, process: &Process) -> Result<()> {
239205
use self::ChangedItem::*;
240206
match self {
241207
AddedFile(path) => utils::remove_file("component", &prefix.abs_path(path))?,
242208
AddedDir(path) => utils::remove_dir("component", &prefix.abs_path(path))?,
243209
RemovedFile(path, tmp) | ModifiedFile(path, Some(tmp)) => {
244-
utils::rename("component", tmp, &prefix.abs_path(path), notify, process)?
210+
utils::rename("component", tmp, &prefix.abs_path(path), process)?
245211
}
246212
RemovedDir(path, tmp) => utils::rename(
247213
"component",
248214
&tmp.join("bk"),
249215
&prefix.abs_path(path),
250-
notify,
251216
process,
252217
)?,
253218
ModifiedFile(path, None) => {
@@ -304,7 +269,6 @@ impl<'a> ChangedItem<'a> {
304269
component: &str,
305270
relpath: PathBuf,
306271
tmp_cx: &'a temp::Context,
307-
notify: &'a dyn Fn(Notification<'_>),
308272
process: &Process,
309273
) -> Result<Self> {
310274
let abs_path = prefix.abs_path(&relpath);
@@ -316,7 +280,7 @@ impl<'a> ChangedItem<'a> {
316280
}
317281
.into())
318282
} else {
319-
utils::rename("component", &abs_path, &backup, notify, process)?;
283+
utils::rename("component", &abs_path, &backup, process)?;
320284
Ok(ChangedItem::RemovedFile(relpath, backup))
321285
}
322286
}
@@ -325,7 +289,6 @@ impl<'a> ChangedItem<'a> {
325289
component: &str,
326290
relpath: PathBuf,
327291
tmp_cx: &'a temp::Context,
328-
notify: &'a dyn Fn(Notification<'_>),
329292
process: &Process,
330293
) -> Result<Self> {
331294
let abs_path = prefix.abs_path(&relpath);
@@ -337,7 +300,7 @@ impl<'a> ChangedItem<'a> {
337300
}
338301
.into())
339302
} else {
340-
utils::rename("component", &abs_path, &backup.join("bk"), notify, process)?;
303+
utils::rename("component", &abs_path, &backup.join("bk"), process)?;
341304
Ok(ChangedItem::RemovedDir(relpath, backup))
342305
}
343306
}
@@ -364,23 +327,21 @@ impl<'a> ChangedItem<'a> {
364327
component: &str,
365328
relpath: PathBuf,
366329
src: &Path,
367-
notify: &'a dyn Fn(Notification<'_>),
368330
process: &Process,
369331
) -> Result<Self> {
370332
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
371-
utils::rename("component", src, &abs_path, notify, process)?;
333+
utils::rename("component", src, &abs_path, process)?;
372334
Ok(ChangedItem::AddedFile(relpath))
373335
}
374336
fn move_dir(
375337
prefix: &InstallPrefix,
376338
component: &str,
377339
relpath: PathBuf,
378340
src: &Path,
379-
notify: &'a dyn Fn(Notification<'_>),
380341
process: &Process,
381342
) -> Result<Self> {
382343
let abs_path = ChangedItem::dest_abs_path(prefix, component, &relpath)?;
383-
utils::rename("component", src, &abs_path, notify, process)?;
344+
utils::rename("component", src, &abs_path, process)?;
384345
Ok(ChangedItem::AddedDir(relpath))
385346
}
386347
}

‎src/dist/download.rs‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -107,13 +107,7 @@ impl<'a> DownloadCfg<'a> {
107107
} else {
108108
(self.notify_handler)(Notification::ChecksumValid(url.as_ref()));
109109

110-
utils::rename(
111-
"downloaded",
112-
&partial_file_path,
113-
&target_file,
114-
self.notify_handler,
115-
self.process,
116-
)?;
110+
utils::rename("downloaded", &partial_file_path, &target_file, self.process)?;
117111
Ok(File { path: target_file })
118112
}
119113
}

‎src/notifications.rs‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,6 @@ pub enum Notification<'a> {
5555
Error(String),
5656
UsingCurl,
5757
UsingReqwest,
58-
/// Renaming encountered a file in use error and is retrying.
59-
/// The InUse aspect is a heuristic - the OS specifies
60-
/// Permission denied, but as we work in users home dirs and
61-
/// running programs like virus scanner are known to cause this
62-
/// the heuristic is quite good.
63-
RenameInUse(&'a Path, &'a Path),
6458
CreatingRoot(&'a Path),
6559
CreatingFile(&'a Path),
6660
FileDeletion(&'a Path, io::Result<()>),
@@ -127,7 +121,6 @@ impl Notification<'_> {
127121
| ResumingPartialDownload
128122
| UsingCurl
129123
| UsingReqwest => NotificationLevel::Debug,
130-
RenameInUse(_, _) => NotificationLevel::Info,
131124
Error(_) => NotificationLevel::Error,
132125
CreatingRoot(_) | CreatingFile(_) => NotificationLevel::Debug,
133126
FileDeletion(_, result) | DirectoryDeletion(_, result) => match result {
@@ -252,12 +245,6 @@ impl Display for Notification<'_> {
252245
SignatureInvalid(url) => write!(f, "Signature verification failed for '{url}'"),
253246
RetryingDownload(url) => write!(f, "retrying download for '{url}'"),
254247
Error(e) => write!(f, "error: '{e}'"),
255-
RenameInUse(src, dest) => write!(
256-
f,
257-
"retrying renaming '{}' to '{}'",
258-
src.display(),
259-
dest.display()
260-
),
261248
SetDefaultBufferSize(size) => write!(
262249
f,
263250
"using up to {} of RAM to unpack components",

‎src/toolchain.rs‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -554,9 +554,7 @@ impl<'a> Toolchain<'a> {
554554
InstalledPath::File { name, path } => {
555555
utils::ensure_file_removed(name, &path)?
556556
}
557-
InstalledPath::Dir { path } => {
558-
install::uninstall(path)?
559-
}
557+
InstalledPath::Dir { path } => install::uninstall(path)?,
560558
}
561559
}
562560
true

‎src/utils/mod.rs‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use std::process::ExitStatus;
1111
use anyhow::{Context, Result, anyhow, bail};
1212
use retry::delay::{Fibonacci, jitter};
1313
use retry::{OperationResult, retry};
14-
use tracing::{debug, warn};
14+
use tracing::{debug, info, warn};
1515
use url::Url;
1616

1717
use crate::errors::*;
@@ -383,17 +383,13 @@ fn copy_and_delete(name: &'static str, src: &Path, dest: &Path) -> Result<()> {
383383
}
384384
}
385385

386-
pub fn rename<'a, N>(
386+
pub fn rename(
387387
name: &'static str,
388-
src: &'a Path,
389-
dest: &'a Path,
390-
notify_handler: &'a dyn Fn(N),
388+
src: &Path,
389+
dest: &Path,
391390
#[allow(unused_variables)] // Only used on Linux
392391
process: &Process,
393-
) -> Result<()>
394-
where
395-
N: From<Notification<'a>>,
396-
{
392+
) -> Result<()> {
397393
// https://github.com/rust-lang/rustup/issues/1870
398394
// 21 fib steps from 1 sums to ~28 seconds, hopefully more than enough
399395
// for our previous poor performance that avoided the race condition with
@@ -406,7 +402,12 @@ where
406402
Ok(()) => OperationResult::Ok(()),
407403
Err(e) => match e.kind() {
408404
io::ErrorKind::PermissionDenied => {
409-
notify_handler(Notification::RenameInUse(src, dest).into());
405+
// Renaming encountered a file in use error and is retrying.
406+
// The InUse aspect is a heuristic - the OS specifies
407+
// Permission denied, but as we work in users home dirs and
408+
// running programs like virus scanner are known to cause this
409+
// the heuristic is quite good.
410+
info!(source = %src.display(), destination = %dest.display(), "renaming file in use, retrying");
410411
OperationResult::Retry(e)
411412
}
412413
#[cfg(target_os = "linux")]

0 commit comments

Comments
 (0)