Skip to content

Commit

Permalink
Stop using Instant for cache age
Browse files Browse the repository at this point in the history
Although monotonic and therefore safe from certain failure modes that
SystemTime arithmetic can encounter, Instant's implementation is not
consistent and can panic in certain implementations
(rust-lang/rust#100141).

Despite the risk of errors I've also (mostly) convinced myself that
SystemTime is the more-correct type to use for this purpose, and my
attempts to avoid it by converting them to Instants are mostly unhelpful
(e.g. if the system time has changed the Instant representations would
be wrong too).

This PR includes a breaking API change to the `CacheStatus` enum, but
should not affect CLI users.
  • Loading branch information
dimo414 committed Jan 13, 2024
1 parent 29ae490 commit 4c5260a
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 15 deletions.
26 changes: 13 additions & 13 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use std::fs::{File, OpenOptions};
use std::hash::{Hash, Hasher};
use std::io::{self, BufReader, ErrorKind, BufWriter, Write, Read};
use std::path::{PathBuf, Path};
use std::time::{Duration, Instant, SystemTime};
use std::time::{Duration, SystemTime};

use anyhow::{anyhow, Context, Error, Result};
use serde::{Serialize, Deserialize};
Expand Down Expand Up @@ -693,8 +693,8 @@ impl Cache {
let found: CacheEntry<K, V> = Cache::deserialize(reader)?;
// Discard data that is too old
let mtime = std::fs::metadata(&path)?.modified()?;
let elapsed = mtime.elapsed();
if elapsed.is_err() || elapsed.unwrap() > max_age {
let elapsed = mtime.elapsed().unwrap_or(Duration::MAX);
if elapsed > max_age {
debug_msg!("lookup {} expired", path.display());
return match std::fs::remove_file(&path) {
Ok(_) => Ok(None),
Expand Down Expand Up @@ -761,7 +761,7 @@ impl Cache {

fn cleanup(&self) -> Result<()> {
fn delete_stale_file(file: &Path, ttl: Duration) -> Result<()> {
let age = std::fs::metadata(file)?.modified()?.elapsed()?;
let age = std::fs::metadata(file)?.modified()?.elapsed().unwrap_or(Duration::MAX);
if age > ttl {
std::fs::remove_file(file)?;
}
Expand All @@ -773,7 +773,7 @@ impl Cache {
// Don't bother if cleanup has been attempted recently
let last_attempt_file = self.cache_dir.join("last_cleanup");
if let Ok(metadata) = last_attempt_file.metadata() {
if metadata.modified()?.elapsed()? < Duration::from_secs(30) {
if metadata.modified()?.elapsed().unwrap_or(Duration::MAX) < Duration::from_secs(30) {
debug_msg!("cleanup skip recent");
return Ok(());
}
Expand Down Expand Up @@ -974,7 +974,7 @@ mod cache_tests {
#[derive(Debug, Copy, Clone)]
pub enum CacheStatus {
/// Command was found in the cache. Contains the time the returned invocation was cached.
Hit(Instant),
Hit(SystemTime),
/// Command was not found in the cache and was executed. Contains the execution time of the
/// subprocess.
Miss(Duration),
Expand Down Expand Up @@ -1118,7 +1118,7 @@ impl Bkt {
use std::process::Stdio;
let command = command.stdout(Stdio::piped()).stderr(Stdio::piped());

let start = Instant::now();
let start = std::time::Instant::now();
let mut child = command.spawn().with_context(|| format!("Failed to run command: {:?}", command))?;

let child_out = child.stdout.take().ok_or(anyhow!("cannot attach to child stdout"))?;
Expand Down Expand Up @@ -1209,13 +1209,13 @@ impl Bkt {
stdout.write_all(inv.stdout())?;
stderr.write_all(inv.stderr())?;
}
(inv, CacheStatus::Hit(Instant::now() - mtime.elapsed()?))
(inv, CacheStatus::Hit(mtime))
},
None => {
let cleanup_hook = self.maybe_cleanup_once();
let start = Instant::now();
let start = std::time::Instant::now();
let result = Bkt::execute_subprocess(&command, output_streams).context("Subprocess execution failed")?;
let runtime = Instant::now() - start;
let runtime = start.elapsed();
if command.persist_failures || result.exit_code == 0 {
self.cache.store(&command, &result, ttl).context("Cache write failed")?;
}
Expand Down Expand Up @@ -1277,14 +1277,14 @@ impl Bkt {
{
let command = command.try_into()?;
let cleanup_hook = self.maybe_cleanup_once();
let start = Instant::now();
let start = std::time::Instant::now();
let result = Bkt::execute_subprocess(&command, output_streams).context("Subprocess execution failed")?;
let elapsed = Instant::now() - start;
let runtime = start.elapsed();
if command.persist_failures || result.exit_code == 0 {
self.cache.store(&command, &result, ttl).context("Cache write failed")?;
}
Bkt::join_cleanup_thread(cleanup_hook);
Ok((result, elapsed))
Ok((result, runtime))
}

/// Clean the cache in the background on a cache-miss; this will usually
Expand Down
4 changes: 2 additions & 2 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::ffi::OsString;
use std::io::{self, stderr, stdout, Write};
use std::path::PathBuf;
use std::process::{Command, exit, Stdio};
use std::time::{Duration, Instant};
use std::time::Duration;

use anyhow::{Context, Result};
use clap::Parser;
Expand Down Expand Up @@ -117,7 +117,7 @@ fn run(cli: Cli) -> Result<i32> {
&command, ttl, DisregardBrokenPipe(Box::new(stdout())), DisregardBrokenPipe(Box::new(stderr())))?;
if let Some(stale) = stale {
if let bkt::CacheStatus::Hit(cached_at) = status {
if (Instant::now() - cached_at) > stale {
if cached_at.elapsed().unwrap_or(Duration::MAX) > stale {
force_update_async()?;
}
}
Expand Down

0 comments on commit 4c5260a

Please sign in to comment.