Auto merge of #705 - Mark-Simulacrum:no-state, r=Mark-Simulacrum

Remove RunnerState Store logs into a local stack variable that gets uploaded at the end. There's no reason to keep them in a semi-global place, and this makes it easier to capture more into the logs (e.g., should make us include retry logs automatically).
rust-lang · Oct 28, 2023 · 0f43df9 · 0f43df9
2 parents afd8d32 + 5715bd3
commit 0f43df9
Show file tree

Hide file tree

Showing 8 changed files with 46 additions and 141 deletions.
diff --git a/src/agent/results.rs b/src/agent/results.rs
@@ -1,5 +1,4 @@
 use crate::agent::api::AgentApi;
-use crate::config::Config;
 use crate::crates::Crate;
 use crate::experiments::Experiment;
 use crate::prelude::*;
@@ -48,16 +47,14 @@ impl<'a> WriteResults for ResultsUploader<'a> {
         ex: &Experiment,
         toolchain: &Toolchain,
         krate: &Crate,
-        existing_logs: Option<LogStorage>,
-        config: &Config,
+        storage: &LogStorage,
         _: EncodingType,
         f: F,
     ) -> Fallible<TestResult>
     where
         F: FnOnce() -> Fallible<TestResult>,
     {
-        let storage = existing_logs.unwrap_or_else(|| LogStorage::from(config));
-        let result = logging::capture(&storage, f)?;
+        let result = logging::capture(storage, f)?;
         let output = storage.to_string();
 
         let mut updated = None;

diff --git a/src/report/archives.rs b/src/report/archives.rs
@@ -190,6 +190,7 @@ mod tests {
     use crate::results::{DatabaseDB, EncodingType, FailureReason, TestResult, WriteResults};
     use flate2::read::GzDecoder;
     use mime::Mime;
+    use rustwide::logging::LogStorage;
     use std::io::Read;
     use tar::Archive;
 
@@ -217,8 +218,7 @@ mod tests {
                 &ex,
                 &ex.toolchains[0],
                 crate1,
-                None,
-                &config,
+                &LogStorage::from(&config),
                 EncodingType::Gzip,
                 || {
                     info!("tc1 crate1");
@@ -231,8 +231,7 @@ mod tests {
                 &ex,
                 &ex.toolchains[1],
                 crate1,
-                None,
-                &config,
+                &LogStorage::from(&config),
                 EncodingType::Plain,
                 || {
                     info!("tc2 crate1");
@@ -245,8 +244,7 @@ mod tests {
                 &ex,
                 &ex.toolchains[0],
                 crate2,
-                None,
-                &config,
+                &LogStorage::from(&config),
                 EncodingType::Gzip,
                 || {
                     info!("tc1 crate2");
@@ -259,8 +257,7 @@ mod tests {
                 &ex,
                 &ex.toolchains[1],
                 crate2,
-                None,
-                &config,
+                &LogStorage::from(&config),
                 EncodingType::Plain,
                 || {
                     info!("tc2 crate2");

diff --git a/src/results/db.rs b/src/results/db.rs
@@ -1,4 +1,3 @@
-use crate::config::Config;
 use crate::crates::Crate;
 use crate::db::{Database, QueryUtils};
 use crate::experiments::{Experiment, Status};
@@ -224,16 +223,14 @@ impl<'a> WriteResults for DatabaseDB<'a> {
         ex: &Experiment,
         toolchain: &Toolchain,
         krate: &Crate,
-        existing_logs: Option<LogStorage>,
-        config: &Config,
+        storage: &LogStorage,
         encoding_type: EncodingType,
         f: F,
     ) -> Fallible<TestResult>
     where
         F: FnOnce() -> Fallible<TestResult>,
     {
-        let storage = existing_logs.unwrap_or_else(|| LogStorage::from(config));
-        let result = logging::capture(&storage, f)?;
+        let result = logging::capture(storage, f)?;
         let output = storage.to_string();
         self.store_result(
             ex,
@@ -266,6 +263,7 @@ impl<'a> DeleteResults for DatabaseDB<'a> {
 #[cfg(test)]
 mod tests {
     use base64::Engine;
+    use rustwide::logging::LogStorage;
 
     use super::{DatabaseDB, ProgressData, TaskResult};
     use crate::actions::{Action, ActionsCtx, CreateExperiment};
@@ -360,8 +358,7 @@ mod tests {
                 &ex,
                 &MAIN_TOOLCHAIN,
                 &krate,
-                None,
-                &config,
+                &LogStorage::from(&config),
                 EncodingType::Plain,
                 || {
                     info!("hello world");
@@ -408,8 +405,7 @@ mod tests {
                 &ex,
                 &TEST_TOOLCHAIN,
                 &krate,
-                None,
-                &config,
+                &LogStorage::from(&config),
                 EncodingType::Plain,
                 || {
                     info!("Another log message!");

diff --git a/src/results/mod.rs b/src/results/mod.rs
@@ -1,7 +1,6 @@
 mod db;
 #[cfg(test)]
 mod dummy;
-use crate::config::Config;
 use crate::crates::Crate;
 use crate::experiments::Experiment;
 use crate::prelude::*;
@@ -45,8 +44,7 @@ pub trait WriteResults {
         ex: &Experiment,
         toolchain: &Toolchain,
         krate: &Crate,
-        existing_logs: Option<LogStorage>,
-        config: &Config,
+        existing_logs: &LogStorage,
         encoding_type: EncodingType,
         f: F,
     ) -> Fallible<TestResult>

diff --git a/src/runner/mod.rs b/src/runner/mod.rs
@@ -9,10 +9,7 @@ use crate::experiments::{Experiment, Mode};
 use crate::prelude::*;
 use crate::results::{TestResult, WriteResults};
 use crate::runner::worker::{DiskSpaceWatcher, Worker};
-use rustwide::logging::LogStorage;
 use rustwide::Workspace;
-use std::collections::HashMap;
-use std::sync::Mutex;
 use std::thread::scope;
 use std::time::Duration;
 
@@ -23,28 +20,6 @@ const DISK_SPACE_WATCHER_THRESHOLD: f32 = 0.80;
 #[error("overridden task result to {0}")]
 pub struct OverrideResult(TestResult);
 
-struct RunnerStateInner {
-    prepare_logs: HashMap<Crate, LogStorage>,
-}
-
-struct RunnerState {
-    inner: Mutex<RunnerStateInner>,
-}
-
-impl RunnerState {
-    fn new() -> Self {
-        RunnerState {
-            inner: Mutex::new(RunnerStateInner {
-                prepare_logs: HashMap::new(),
-            }),
-        }
-    }
-
-    fn lock(&self) -> std::sync::MutexGuard<RunnerStateInner> {
-        self.inner.lock().unwrap()
-    }
-}
-
 pub fn run_ex<DB: WriteResults + Sync>(
     ex: &Experiment,
     workspace: &Workspace,
@@ -106,19 +81,8 @@ pub fn run_ex<DB: WriteResults + Sync>(
 
     info!("running tasks in {} threads...", threads_count);
 
-    let state = RunnerState::new();
     let workers = (0..threads_count)
-        .map(|i| {
-            Worker::new(
-                format!("worker-{i}"),
-                workspace,
-                ex,
-                config,
-                &state,
-                db,
-                next_crate,
-            )
-        })
+        .map(|i| Worker::new(format!("worker-{i}"), workspace, ex, config, db, next_crate))
         .collect::<Vec<_>>();
 
     let disk_watcher = DiskSpaceWatcher::new(

diff --git a/src/runner/tasks.rs b/src/runner/tasks.rs
@@ -3,8 +3,8 @@ use crate::crates::{Crate, GitHubRepo};
 use crate::experiments::Experiment;
 use crate::prelude::*;
 use crate::results::{EncodingType, TestResult, WriteResults};
+use crate::runner::test;
 use crate::runner::test::detect_broken;
-use crate::runner::{test, RunnerState};
 use crate::toolchain::Toolchain;
 use crate::utils;
 use rustwide::{Build, BuildDirectory, Workspace};
@@ -21,7 +21,6 @@ pub(super) struct TaskCtx<'ctx, DB: WriteResults + 'ctx> {
     pub(super) experiment: &'ctx Experiment,
     pub(super) toolchain: &'ctx Toolchain,
     pub(super) krate: &'ctx Crate,
-    pub(super) state: &'ctx RunnerState,
     pub(super) quiet: bool,
 }
 
@@ -33,7 +32,6 @@ impl<'ctx, DB: WriteResults + 'ctx> TaskCtx<'ctx, DB> {
         experiment: &'ctx Experiment,
         toolchain: &'ctx Toolchain,
         krate: &'ctx Crate,
-        state: &'ctx RunnerState,
         quiet: bool,
     ) -> Self {
         TaskCtx {
@@ -43,7 +41,6 @@ impl<'ctx, DB: WriteResults + 'ctx> TaskCtx<'ctx, DB> {
             experiment,
             toolchain,
             krate,
-            state,
             quiet,
         }
     }
@@ -102,10 +99,9 @@ impl Task {
         &self,
         ex: &Experiment,
         db: &DB,
-        state: &RunnerState,
-        config: &Config,
         err: &failure::Error,
         result: &TestResult,
+        storage: &LogStorage,
     ) -> Fallible<()> {
         match self.step {
             TaskStep::Prepare | TaskStep::Cleanup => {}
@@ -116,24 +112,11 @@ impl Task {
             | TaskStep::Clippy { ref tc, .. }
             | TaskStep::Rustdoc { ref tc, .. }
             | TaskStep::UnstableFeatures { ref tc } => {
-                let log_storage = state
-                    .lock()
-                    .prepare_logs
-                    .get(&self.krate)
-                    .map(|s| s.duplicate());
-                db.record_result(
-                    ex,
-                    tc,
-                    &self.krate,
-                    log_storage,
-                    config,
-                    EncodingType::Plain,
-                    || {
-                        error!("this task or one of its parent failed!");
-                        utils::report_failure(err);
-                        Ok(result.clone())
-                    },
-                )?;
+                db.record_result(ex, tc, &self.krate, storage, EncodingType::Plain, || {
+                    error!("this task or one of its parent failed!");
+                    utils::report_failure(err);
+                    Ok(result.clone())
+                })?;
             }
         }
 
@@ -147,7 +130,7 @@ impl Task {
         build_dir: &'ctx HashMap<&'ctx crate::toolchain::Toolchain, Mutex<BuildDirectory>>,
         ex: &'ctx Experiment,
         db: &'ctx DB,
-        state: &'ctx RunnerState,
+        logs: &LogStorage,
     ) -> Fallible<()> {
         let (build_dir, action, test, toolchain, quiet): (
             _,
@@ -184,22 +167,16 @@ impl Task {
             ),
             TaskStep::Cleanup => {
                 // Remove stored logs
-                state.lock().prepare_logs.remove(&self.krate);
                 return Ok(());
             }
             TaskStep::Prepare => {
-                let storage = LogStorage::from(config);
-                state
-                    .lock()
-                    .prepare_logs
-                    .insert(self.krate.clone(), storage.clone());
-                logging::capture(&storage, || {
+                logging::capture(logs, || {
                     let rustwide_crate = self.krate.to_rustwide();
                     for attempt in 1..=15 {
                         match detect_broken(rustwide_crate.fetch(workspace)) {
                             Ok(()) => break,
                             Err(e) => {
-                                if storage.to_string().contains("No space left on device") {
+                                if logs.to_string().contains("No space left on device") {
                                     if attempt == 15 {
                                         // If we've failed 15 times, then
                                         // just give up. It's been at least
@@ -256,33 +233,16 @@ impl Task {
                 // If a skipped crate is somehow sent to the agent (for example, when a crate was
                 // added to the experiment and *then* blacklisted) report the crate as skipped
                 // instead of silently ignoring it.
-                db.record_result(
-                    ex,
-                    tc,
-                    &self.krate,
-                    None,
-                    config,
-                    EncodingType::Plain,
-                    || {
-                        warn!("crate skipped");
-                        Ok(TestResult::Skipped)
-                    },
-                )?;
+                db.record_result(ex, tc, &self.krate, logs, EncodingType::Plain, || {
+                    warn!("crate skipped");
+                    Ok(TestResult::Skipped)
+                })?;
                 return Ok(());
             }
         };
 
-        let ctx = TaskCtx::new(
-            build_dir,
-            config,
-            db,
-            ex,
-            toolchain,
-            &self.krate,
-            state,
-            quiet,
-        );
-        test::run_test(action, &ctx, test)?;
+        let ctx = TaskCtx::new(build_dir, config, db, ex, toolchain, &self.krate, quiet);
+        test::run_test(action, &ctx, test, logs)?;
 
         Ok(())
     }

diff --git a/src/runner/test.rs b/src/runner/test.rs
@@ -10,6 +10,7 @@ use docsrs_metadata::Metadata as DocsrsMetadata;
 use failure::Error;
 use remove_dir_all::remove_dir_all;
 use rustwide::cmd::{CommandError, ProcessLinesActions, SandboxBuilder};
+use rustwide::logging::LogStorage;
 use rustwide::{Build, PrepareError};
 use std::collections::{BTreeSet, HashMap, HashSet};
 use std::convert::TryFrom;
@@ -215,25 +216,19 @@ pub(super) fn run_test<DB: WriteResults>(
     action: &str,
     ctx: &TaskCtx<DB>,
     test_fn: fn(&TaskCtx<DB>, &Build, &[Package]) -> Fallible<TestResult>,
+    logs: &LogStorage,
 ) -> Fallible<()> {
     if let Some(res) = ctx
         .db
         .get_result(ctx.experiment, ctx.toolchain, ctx.krate)?
     {
         info!("skipping crate {}. existing result: {}", ctx.krate, res);
     } else {
-        let log_storage = ctx
-            .state
-            .lock()
-            .prepare_logs
-            .get(ctx.krate)
-            .map(|s| s.duplicate());
         ctx.db.record_result(
             ctx.experiment,
             ctx.toolchain,
             ctx.krate,
-            log_storage,
-            ctx.config,
+            logs,
             EncodingType::Plain,
             || {
                 info!(