Refactor DocFS to fix error handling bugs

Author: P1n3appl3

src/librustdoc/docfs.rs

@@ -13,8 +13,7 @@ use std::fs;
 use std::io;
 use std::path::Path;
 use std::string::ToString;
-use std::sync::mpsc::{channel, Receiver, Sender};
-use std::sync::Arc;
+use std::sync::mpsc::Sender;
 
 macro_rules! try_err {
     ($e:expr, $file:expr) => {
@@ -31,47 +30,24 @@ pub trait PathError {
         S: ToString + Sized;
 }
 
-pub struct ErrorStorage {
-    sender: Option<Sender<Option<String>>>,
-    receiver: Receiver<Option<String>>,
-}
-
-impl ErrorStorage {
-    pub fn new() -> ErrorStorage {
-        let (sender, receiver) = channel();
-        ErrorStorage { sender: Some(sender), receiver }
-    }
-
-    /// Prints all stored errors. Returns the number of printed errors.
-    pub fn write_errors(&mut self, diag: &rustc_errors::Handler) -> usize {
-        let mut printed = 0;
-        // In order to drop the sender part of the channel.
-        self.sender = None;
-
-        for msg in self.receiver.iter() {
-            if let Some(ref error) = msg {
-                diag.struct_err(&error).emit();
-                printed += 1;
-            }
-        }
-        printed
-    }
-}
-
 pub struct DocFS {
     sync_only: bool,
-    errors: Arc<ErrorStorage>,
+    errors: Option<Sender<String>>,
 }
 
 impl DocFS {
-    pub fn new(errors: &Arc<ErrorStorage>) -> DocFS {
-        DocFS { sync_only: false, errors: Arc::clone(errors) }
+    pub fn new(errors: &Sender<String>) -> DocFS {
+        DocFS { sync_only: false, errors: Some(errors.clone()) }
     }
 
     pub fn set_sync_only(&mut self, sync_only: bool) {
         self.sync_only = sync_only;
     }
 
+    pub fn close(&mut self) {
+        self.errors = None;
+    }
+
     pub fn create_dir_all<P: AsRef<Path>>(&self, path: P) -> io::Result<()> {
         // For now, dir creation isn't a huge time consideration, do it
         // synchronously, which avoids needing ordering between write() actions
@@ -88,20 +64,15 @@ impl DocFS {
         if !self.sync_only && cfg!(windows) {
             // A possible future enhancement after more detailed profiling would
             // be to create the file sync so errors are reported eagerly.
-            let contents = contents.as_ref().to_vec();
             let path = path.as_ref().to_path_buf();
-            let sender = self.errors.sender.clone().unwrap();
-            rayon::spawn(move || match fs::write(&path, &contents) {
-                Ok(_) => {
-                    sender.send(None).unwrap_or_else(|_| {
-                        panic!("failed to send error on \"{}\"", path.display())
-                    });
-                }
-                Err(e) => {
-                    sender.send(Some(format!("\"{}\": {}", path.display(), e))).unwrap_or_else(
-                        |_| panic!("failed to send non-error on \"{}\"", path.display()),
-                    );
-                }
+            let contents = contents.as_ref().to_vec();
+            let sender = self.errors.clone().expect("can't write after closing");
+            rayon::spawn(move || {
+                fs::write(&path, contents).unwrap_or_else(|e| {
+                    sender
+                        .send(format!("\"{}\": {}", path.display(), e))
+                        .expect(&format!("failed to send error on \"{}\"", path.display()));
+                });
             });
             Ok(())
         } else {

src/librustdoc/html/render/mod.rs

@@ -44,6 +44,7 @@ use std::path::{Component, Path, PathBuf};
 use std::rc::Rc;
 use std::str;
 use std::string::ToString;
+use std::sync::mpsc::{channel, Receiver};
 use std::sync::Arc;
 
 use itertools::Itertools;
@@ -65,7 +66,7 @@ use serde::{Serialize, Serializer};
 use crate::clean::{self, AttributesExt, Deprecation, GetDefId, SelfTy, TypeKind};
 use crate::config::RenderInfo;
 use crate::config::RenderOptions;
-use crate::docfs::{DocFS, ErrorStorage, PathError};
+use crate::docfs::{DocFS, PathError};
 use crate::doctree;
 use crate::error::Error;
 use crate::formats::cache::{cache, Cache};
@@ -113,7 +114,9 @@ crate struct Context {
     id_map: Rc<RefCell<IdMap>>,
     pub shared: Arc<SharedContext>,
     all: Rc<RefCell<AllTypes>>,
-    pub errors: Arc<ErrorStorage>,
+    /// Storage for the errors produced while generating documentation so they
+    /// can be printed together at the end.
+    pub errors: Rc<Receiver<String>>,
 }
 
 crate struct SharedContext {
@@ -403,7 +406,6 @@ impl FormatRenderer for Context {
             },
             _ => PathBuf::new(),
         };
-        let errors = Arc::new(ErrorStorage::new());
         // If user passed in `--playground-url` arg, we fill in crate name here
         let mut playground = None;
         if let Some(url) = playground_url {
@@ -447,6 +449,7 @@ impl FormatRenderer for Context {
                 }
             }
         }
+        let (sender, receiver) = channel();
         let mut scx = SharedContext {
             collapsed: krate.collapsed,
             src_root,
@@ -459,7 +462,7 @@ impl FormatRenderer for Context {
             style_files,
             resource_suffix,
             static_root_path,
-            fs: DocFS::new(&errors),
+            fs: DocFS::new(&sender),
             edition,
             codes: ErrorCodes::from(UnstableFeatures::from_environment().is_nightly_build()),
             playground,
@@ -493,7 +496,7 @@ impl FormatRenderer for Context {
             id_map: Rc::new(RefCell::new(id_map)),
             shared: Arc::new(scx),
             all: Rc::new(RefCell::new(AllTypes::new())),
-            errors,
+            errors: Rc::new(receiver),
         };
 
         CURRENT_DEPTH.with(|s| s.set(0));
@@ -506,8 +509,8 @@ impl FormatRenderer for Context {
     }
 
     fn after_run(&mut self, diag: &rustc_errors::Handler) -> Result<(), Error> {
-        let nb_errors =
-            Arc::get_mut(&mut self.errors).map_or_else(|| 0, |errors| errors.write_errors(diag));
+        Arc::get_mut(&mut self.shared).unwrap().fs.close();
+        let nb_errors = self.errors.iter().map(|err| diag.struct_err(&err).emit()).count();
         if nb_errors > 0 {
             Err(Error::new(io::Error::new(io::ErrorKind::Other, "I/O error"), ""))
         } else {

src/librustdoc/lib.rs

@@ -507,9 +507,14 @@ fn main_options(options: config::Options) -> i32 {
         ) {
             Ok(_) => rustc_driver::EXIT_SUCCESS,
             Err(e) => {
-                diag.struct_err(&format!("couldn't generate documentation: {}", e.error))
-                    .note(&format!("failed to create or modify \"{}\"", e.file.display()))
-                    .emit();
+                let mut msg =
+                    diag.struct_err(&format!("couldn't generate documentation: {}", e.error));
+                let file = e.file.display().to_string();
+                if file.is_empty() {
+                    msg.emit()
+                } else {
+                    msg.note(&format!("failed to create or modify \"{}\"", file)).emit()
+                }
                 rustc_driver::EXIT_FAILURE
             }
         }