Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make util::Progress thread-safe as prerequisite of #11448 #11602

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ libgit2-sys = "=0.14.1"
memchr = "2.1.3"
opener = "0.5"
os_info = "3.5.0"
parking_lot = "0.12.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you say why this needs parking_lot?
I'm generally reluctant to bring in new dependencies unless there is a strong reason. They increase build times, increase build and porting hazards, and potentially increase maintenance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #11448

It's both. I used std::sync::Mutex in an earlier iteration but found myself writing .unwrap() or .expect("thread didn't panic") which seemed worse than using parking_lot explicitly which is already part of the dependency tree.

Except I just looked and do not see parking_lot in the dependency tree

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think this might have come up in the other PR but wasn't addressed.

I chose it to avoid dealing with lock poisoning. Not using it either meant that fn -> T has to become fn -> Result<T> where ever the lock is obtained, or that I'd have to use unwrap() or expect("no panic in thread"), both of which seemed like the something to avoid.

Is there other options you see, or a choice you would make from the alternatives presented here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except I just looked and do not see parking_lot in the dependency tree

It's coming in with gitoxide.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically for dealing with that I just call .unwrap(). The chance of having a poisoned lock is small if you keep the locked region small, and avoid other panics within it. And the consequences are usually small, unless you try to lock a mutex in a drop() method, which I would usually not recommend (though it looks like this PR does that).

It is certainly not ideal, and there has been a lot of discussion on whether or not to actually try to introduce a different API. There is risk with the design of parking_lot since invariants may no longer hold if some locked region only partially completed its work.

I think it should probably be OK to bring it in if it will be required by gitoxide.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should probably be OK to bring it in if it will be required by gitoxide.

I left parking_lot in for now but will be happy to change to an std::sync::Mutex with unwrap() if there are any more concerns. Just because gitoxide uses parking_lot in some capacity doesn't mean it has to be used here - there are good reasons for lock poisoning and I don't understand the tradeoffs that parking_lot made, but merely chose the simple route assuming that it's probably alright if so many others do it - clearly some sort of fallacy.

pasetors = { version = "0.6.4", features = ["v3", "paserk", "std", "serde"] }
pathdiff = "0.2"
percent-encoding = "2.0"
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/job_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ struct DrainState<'cfg> {
documented: HashSet<PackageId>,
scraped: HashSet<PackageId>,
counts: HashMap<PackageId, usize>,
progress: Progress<'cfg>,
progress: Progress,
next_id: u32,
timings: Timings<'cfg>,

Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ pub struct Downloads<'a, 'cfg> {
/// The next ID to use for creating a token (see `Download::token`).
next: usize,
/// Progress bar.
progress: RefCell<Option<Progress<'cfg>>>,
progress: RefCell<Option<Progress>>,
/// Number of downloads that have successfully finished.
downloads_finished: usize,
/// Total bytes for all successfully downloaded packages.
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl fmt::Debug for Shell {
/// A `Write`able object, either with or without color support
enum ShellOut {
/// A plain write object without color support
Write(Box<dyn Write>),
Write(Box<dyn Write + Send>),
/// Color-enabled stdio, with information on whether color should be used
Stream {
stdout: StandardStream,
Expand Down Expand Up @@ -114,7 +114,7 @@ impl Shell {
}

/// Creates a shell from a plain writable object, with no color, and max verbosity.
pub fn from_write(out: Box<dyn Write>) -> Shell {
pub fn from_write(out: Box<dyn Write + Send>) -> Shell {
Shell {
output: ShellOut::Write(out),
verbosity: Verbosity::Verbose,
Expand Down
20 changes: 10 additions & 10 deletions src/cargo/ops/cargo_clean.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,14 +315,14 @@ trait CleaningProgressBar {
fn on_clean(&mut self) -> CargoResult<()>;
}

struct CleaningFolderBar<'cfg> {
bar: Progress<'cfg>,
struct CleaningFolderBar {
bar: Progress,
max: usize,
cur: usize,
}

impl<'cfg> CleaningFolderBar<'cfg> {
fn new(cfg: &'cfg Config, max: usize) -> Self {
impl CleaningFolderBar {
fn new(cfg: &Config, max: usize) -> Self {
Self {
bar: Progress::with_style("Cleaning", ProgressStyle::Percentage, cfg),
max,
Expand All @@ -335,7 +335,7 @@ impl<'cfg> CleaningFolderBar<'cfg> {
}
}

impl<'cfg> CleaningProgressBar for CleaningFolderBar<'cfg> {
impl CleaningProgressBar for CleaningFolderBar {
fn display_now(&mut self) -> CargoResult<()> {
self.bar.tick_now(self.cur_progress(), self.max, "")
}
Expand All @@ -346,16 +346,16 @@ impl<'cfg> CleaningProgressBar for CleaningFolderBar<'cfg> {
}
}

struct CleaningPackagesBar<'cfg> {
bar: Progress<'cfg>,
struct CleaningPackagesBar {
bar: Progress,
max: usize,
cur: usize,
num_files_folders_cleaned: usize,
package_being_cleaned: String,
}

impl<'cfg> CleaningPackagesBar<'cfg> {
fn new(cfg: &'cfg Config, max: usize) -> Self {
impl CleaningPackagesBar {
fn new(cfg: &Config, max: usize) -> Self {
Self {
bar: Progress::with_style("Cleaning", ProgressStyle::Ratio, cfg),
max,
Expand Down Expand Up @@ -384,7 +384,7 @@ impl<'cfg> CleaningPackagesBar<'cfg> {
}
}

impl<'cfg> CleaningProgressBar for CleaningPackagesBar<'cfg> {
impl CleaningProgressBar for CleaningPackagesBar {
fn display_now(&mut self) -> CargoResult<()> {
self.bar
.tick_now(self.cur_progress(), self.max, &self.format_message())
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/sources/registry/http_remote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub struct Downloads<'cfg> {
/// The next ID to use for creating a token (see `Download::token`).
next: usize,
/// Progress bar.
progress: RefCell<Option<Progress<'cfg>>>,
progress: RefCell<Option<Progress>>,
/// Number of downloads that have successfully finished.
downloads_finished: usize,
/// Number of times the caller has requested blocking. This is used for
Expand Down
24 changes: 16 additions & 8 deletions src/cargo/util/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
//! translate from `ConfigValue` and environment variables to the caller's
//! desired type.

use parking_lot::{Mutex, MutexGuard};
use std::borrow::Cow;
use std::cell::{RefCell, RefMut};
use std::collections::hash_map::Entry::{Occupied, Vacant};
Expand All @@ -62,7 +63,7 @@ use std::io::{self, SeekFrom};
use std::mem;
use std::path::{Path, PathBuf};
use std::str::FromStr;
use std::sync::Once;
use std::sync::{Arc, Once};
use std::time::Instant;

use self::ConfigValue as CV;
Expand Down Expand Up @@ -156,7 +157,7 @@ pub struct Config {
/// The location of the user's Cargo home directory. OS-dependent.
home_path: Filesystem,
/// Information about how to write messages to the shell
shell: RefCell<Shell>,
shell: Arc<Mutex<Shell>>,
/// A collection of configuration options
values: LazyCell<HashMap<String, ConfigValue>>,
/// A collection of configuration options from the credentials file
Expand Down Expand Up @@ -282,7 +283,7 @@ impl Config {

Config {
home_path: Filesystem::new(homedir),
shell: RefCell::new(shell),
shell: Arc::new(Mutex::new(shell)),
cwd,
search_stop_path: None,
values: LazyCell::new(),
Expand Down Expand Up @@ -393,8 +394,17 @@ impl Config {
}

/// Gets a reference to the shell, e.g., for writing error messages.
pub fn shell(&self) -> RefMut<'_, Shell> {
self.shell.borrow_mut()
///
/// # Deadlock Warning
///
/// A deadlock will occour if a thread calls this method while still holding the guard returned in the previous call.
pub fn shell(&self) -> MutexGuard<'_, Shell> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the docstring here include some warnings about the restrictions the caller should be careful about? For example, this should not be called in the same thread if that thread already has a shell (otherwise it would deadlock).

Overall I'm a bit nervous about this since there won't be any compile time checks, and there are a lot of places that get a shell. I don't think I can review all of those call sites. I think the current code is probably ok, otherwise the RefCel would have panic'ed, but a panic is a lot better than a deadlock.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good call, and I see the issue with going RefCell to Mutex as well. So much so that I'd also love to not have to do that.

For now I have added a big deadlock notice.

self.shell.lock()
}

/// Gets a shared reference to the shell, e.g., for writing error messages, for use when writing from threads.
pub fn shell_detached(&self) -> Arc<Mutex<Shell>> {
Arc::clone(&self.shell)
}

/// Gets the path to the `rustdoc` executable.
Expand Down Expand Up @@ -1286,9 +1296,7 @@ impl Config {
// --config path_to_file
let str_path = arg_as_path
.to_str()
.ok_or_else(|| {
anyhow::format_err!("config path {:?} is not utf-8", arg_as_path)
})?
.ok_or_else(|| format_err!("config path {:?} is not utf-8", arg_as_path))?
.to_string();
self._load_file(&self.cwd().join(&str_path), &mut seen, true, WhyLoad::Cli)
.with_context(|| format!("failed to load config from `{}`", str_path))?
Expand Down
48 changes: 27 additions & 21 deletions src/cargo/util/progress.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
use parking_lot::Mutex;
use std::cmp;
use std::env;
use std::sync::Arc;
use std::time::{Duration, Instant};

use crate::core::shell::Verbosity;
use crate::core::Shell;
use crate::util::config::ProgressWhen;
use crate::util::{CargoResult, Config};
use cargo_util::is_ci;
use unicode_width::UnicodeWidthChar;

pub struct Progress<'cfg> {
state: Option<State<'cfg>>,
pub struct Progress {
state: Option<State>,
}

pub enum ProgressStyle {
Expand All @@ -23,8 +26,8 @@ struct Throttle {
last_update: Instant,
}

struct State<'cfg> {
config: &'cfg Config,
struct State {
shell: Arc<Mutex<Shell>>,
format: Format,
name: String,
done: bool,
Expand All @@ -39,8 +42,8 @@ struct Format {
max_print: usize,
}

impl<'cfg> Progress<'cfg> {
pub fn with_style(name: &str, style: ProgressStyle, cfg: &'cfg Config) -> Progress<'cfg> {
impl Progress {
pub fn with_style(name: &str, style: ProgressStyle, cfg: &Config) -> Progress {
// report no progress when -q (for quiet) or TERM=dumb are set
// or if running on Continuous Integration service like Travis where the
// output logs get mangled.
Expand All @@ -60,15 +63,15 @@ impl<'cfg> Progress<'cfg> {
Progress::new_priv(name, style, cfg)
}

fn new_priv(name: &str, style: ProgressStyle, cfg: &'cfg Config) -> Progress<'cfg> {
fn new_priv(name: &str, style: ProgressStyle, cfg: &Config) -> Progress {
let progress_config = cfg.progress_config();
let width = progress_config
.width
.or_else(|| cfg.shell().err_width().progress_max_width());

Progress {
state: width.map(|n| State {
config: cfg,
shell: cfg.shell_detached(),
format: Format {
style,
max_width: n,
Expand All @@ -93,7 +96,7 @@ impl<'cfg> Progress<'cfg> {
self.state.is_some()
}

pub fn new(name: &str, cfg: &'cfg Config) -> Progress<'cfg> {
pub fn new(name: &str, cfg: &Config) -> Progress {
Self::with_style(name, ProgressStyle::Percentage, cfg)
}

Expand Down Expand Up @@ -180,7 +183,7 @@ impl Throttle {
}
}

impl<'cfg> State<'cfg> {
impl State {
fn tick(&mut self, cur: usize, max: usize, msg: &str) -> CargoResult<()> {
if self.done {
return Ok(());
Expand Down Expand Up @@ -215,29 +218,32 @@ impl<'cfg> State<'cfg> {
}

// Only update if the line has changed.
if self.config.shell().is_cleared() || self.last_line.as_ref() != Some(&line) {
let mut shell = self.config.shell();
shell.set_needs_clear(false);
shell.status_header(&self.name)?;
write!(shell.err(), "{}\r", line)?;
self.last_line = Some(line);
shell.set_needs_clear(true);
{
let mut shell = self.shell.lock();
if shell.is_cleared() || self.last_line.as_ref() != Some(&line) {
shell.set_needs_clear(false);
shell.status_header(&self.name)?;
write!(shell.err(), "{}\r", line)?;
self.last_line = Some(line);
shell.set_needs_clear(true);
}
}

Ok(())
}

fn clear(&mut self) {
// No need to clear if the progress is not currently being displayed.
if self.last_line.is_some() && !self.config.shell().is_cleared() {
self.config.shell().err_erase_line();
let mut shell = self.shell.lock();
if self.last_line.is_some() && !shell.is_cleared() {
shell.err_erase_line();
self.last_line = None;
}
}

fn try_update_max_width(&mut self) {
if self.fixed_width.is_none() {
if let Some(n) = self.config.shell().err_width().progress_max_width() {
if let Some(n) = self.shell.lock().err_width().progress_max_width() {
self.format.max_width = n;
}
}
Expand Down Expand Up @@ -323,7 +329,7 @@ impl Format {
}
}

impl<'cfg> Drop for State<'cfg> {
impl Drop for State {
fn drop(&mut self) {
self.clear();
}
Expand Down