-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}; | ||
|
@@ -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; | ||
|
@@ -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 | ||
|
@@ -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(), | ||
|
@@ -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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good call, and I see the issue with going 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. | ||
|
@@ -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))? | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From #11448
Except I just looked and do not see
parking_lot
in the dependency treeThere was a problem hiding this comment.
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 becomefn -> Result<T>
where ever the lock is obtained, or that I'd have to useunwrap()
orexpect("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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's coming in with
gitoxide
.There was a problem hiding this comment.
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 adrop()
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left
parking_lot
in for now but will be happy to change to anstd::sync::Mutex
withunwrap()
if there are any more concerns. Just becausegitoxide
usesparking_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 thatparking_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.