From f2d656a85150e47839606be6623ff5d12cdb7df3 Mon Sep 17 00:00:00 2001 From: Barret Rennie Date: Fri, 17 Jul 2020 16:01:53 -0400 Subject: [PATCH] Cleanup completed resumed requests The runner now will cleanup resumed requests as they are completed (either successfully or unsucessfully). Additionally, the `FsRequestManager` will cleanup any requests that it cannot resume. --- fxrunner/src/bin/main.rs | 2 +- fxrunner/src/lib/proto.rs | 32 ++++++++++++++-------------- fxrunner/src/lib/request.rs | 42 ++++++++++++++++++++++++++++++------- 3 files changed, 51 insertions(+), 25 deletions(-) diff --git a/fxrunner/src/bin/main.rs b/fxrunner/src/bin/main.rs index 34bc84d..90364f1 100644 --- a/fxrunner/src/bin/main.rs +++ b/fxrunner/src/bin/main.rs @@ -88,7 +88,7 @@ async fn fxrunner(log: Logger, options: Options, config: Config) -> Result<(), B shutdown_provider(&options), FirefoxCi::default(), WindowsPerfProvider::default(), - FsRequestManager::new(&config.requests_dir), + FsRequestManager::new(log.clone(), &config.requests_dir), ) .await? { diff --git a/fxrunner/src/lib/proto.rs b/fxrunner/src/lib/proto.rs index 335bf80..9fdd74a 100644 --- a/fxrunner/src/lib/proto.rs +++ b/fxrunner/src/lib/proto.rs @@ -18,7 +18,9 @@ use tokio::task::spawn_blocking; use crate::fs::PathExt; use crate::osapi::{cpu_and_disk_idle, PerfProvider, ShutdownProvider, WaitForIdleError}; -use crate::request::{NewRequestError, RequestInfo, RequestManager, ResumeRequestError}; +use crate::request::{ + cleanup_request, NewRequestError, RequestInfo, RequestManager, ResumeRequestError, +}; use crate::taskcluster::Taskcluster; use crate::zip::{unzip, ZipError}; @@ -85,14 +87,7 @@ where } }; - let cleanup = guard( - (request_info.clone(), self.log.clone()), - |(request_info, log)| { - if let Err(e) = std::fs::remove_dir_all(&request_info.path) { - error!(log, "Could not cleanup request"; "request_id" => %request_info.id, "error" => ?e); - } - }, - ); + let cleanup = guard(self.log.clone(), |log| cleanup_request(log, &request_info)); self.send(NewRequestResponse { request_id: Ok(request_info.id.clone().into_owned()), @@ -187,13 +182,18 @@ where ) -> Result<(), RunnerProtoError> { info!(self.log, "Received resumption request"); - if let Err(e) = self.request_manager.resume_request(&request.id).await { - self.send(ResumeResponse { - result: Err(e.into_error_message()), - }) - .await?; - return Err(e.into()); - } + let request_info = match self.request_manager.resume_request(&request.id).await { + Ok(request_info) => request_info, + Err(e) => { + self.send(ResumeResponse { + result: Err(e.into_error_message()), + }) + .await?; + return Err(e.into()); + } + }; + + let _cleanup = guard(self.log.clone(), |log| cleanup_request(log, &request_info)); self.send(ResumeResponse { result: Ok(()) }).await?; diff --git a/fxrunner/src/lib/request.rs b/fxrunner/src/lib/request.rs index 747015f..dc59668 100644 --- a/fxrunner/src/lib/request.rs +++ b/fxrunner/src/lib/request.rs @@ -10,6 +10,8 @@ use std::path::{Path, PathBuf}; use async_trait::async_trait; use rand::distributions::Alphanumeric; use rand::prelude::*; +use scopeguard::{guard, ScopeGuard}; +use slog::error; use thiserror::Error; use tokio::fs::create_dir; @@ -46,12 +48,16 @@ pub trait RequestManager { } pub struct FsRequestManager { + log: slog::Logger, path: PathBuf, } impl FsRequestManager { - pub fn new(path: &Path) -> Self { - FsRequestManager { path: path.into() } + pub fn new(log: slog::Logger, path: &Path) -> Self { + FsRequestManager { + log, + path: path.into(), + } } } @@ -119,14 +125,21 @@ impl RequestManager for FsRequestManager { }); } - if !path.join("profile").is_dir_async().await { + let request_info = RequestInfo { + path, + id: Cow::Borrowed(request_id), + }; + + let cleanup = guard(self.log.clone(), |log| cleanup_request(log, &request_info)); + + if !request_info.path.join("profile").is_dir_async().await { return Err(ResumeRequestError { kind: ResumeRequestErrorKind::MissingProfile, request_id: request_id.into(), }); } - let firefox_path = path.join("firefox"); + let firefox_path = request_info.path.join("firefox"); let bin_path = firefox_path.join("firefox.exe"); if !firefox_path.is_dir_async().await || !bin_path.is_file_async().await { return Err(ResumeRequestError { @@ -135,10 +148,8 @@ impl RequestManager for FsRequestManager { }); } - Ok(RequestInfo { - path, - id: Cow::Borrowed(request_id), - }) + drop(ScopeGuard::into_inner(cleanup)); + Ok(request_info) } async fn ensure_valid_profile_dir<'a>( @@ -186,3 +197,18 @@ pub enum NewRequestError { fn validate_request_id(request_id: &str) -> bool { request_id.len() == REQUEST_ID_LEN && request_id.chars().all(|c| c.is_ascii_alphanumeric()) } + +/// Synchronously cleanup a request given by the request info. +pub fn cleanup_request(log: slog::Logger, request_info: &RequestInfo<'_>) { + // This must be performed synchronously because there is no async version of + // the drop trait. + // + // A future could be spawned that would trigger when the guard goes out of + // scope, but we cannot `await` its completion. + // + // Having a synchronous operation in the failure case seems like an okay + // compromise. + if let Err(e) = std::fs::remove_dir_all(&request_info.path) { + error!(log, "Could not cleanup request"; "request_id" => %request_info.id, "error" => %e); + } +}