Conversation
|
I'm hesitant to add complexity here, what is the problem with |
|
With some recent improvements that will hopefully land soon in std, using parking lot to eek out a bit of extra performance won't be necessary. Furthermore, parking lot actually backs their implementation with a region of memory on the heap (called the parking lot) to take care of storing metadata. It's not as lightweight as it may seem. |
|
Those seem like great improvements, I'm open to this. One thing that I think can be improved on in this impl is how to handle a poisoned lock. In this context, I think we could probably recover from a poisoned lock. |
|
That was my thought as well, I just was unsure of the implications of always returning the lock after poisoning. For instance it could easily be changed to this: match self.0.lock() {
Ok(guard) => guard,
Err(error) => error.into_inner(),
}A couple notes on this: are there performance implications for using a poisoned lock as normal? Are we sure that we will always have logically correct behavior regardless of poisoning? I'll think about these issues and post any updates, but I'd like to hear your thoughts too. |
|
The part where it said "Any future attempts to lock the Mutex will return an There are a couple places where we hold a guard across a call to I'm personally in favor of ignoring poison errors and changing relevant documentation to say "if this method panics, then it still could be called again." If you're fine with that I'll go ahead and render it. |
| let &(ref lock, ref cvar) = &*self.cond_pair.clone(); | ||
| let mut ready = lock.lock(); | ||
| let mut ready = lock.lock()?; | ||
| if !*ready { |
There was a problem hiding this comment.
I believe this would have spurious wakeups with the stdlib condvar.
| #[allow(dead_code)] | ||
| pub(crate) mod sync { | ||
| #[derive(Debug)] | ||
| pub struct Mutex<T: ?Sized>(std::sync::Mutex<T>); |
There was a problem hiding this comment.
this should probably also just be a pub type like condvar
2 participants
Currently, if you use the
file_appenderorrolling_file_appenderfeatures then this pullsparking_lotinto the dependency tree. This PR would keepparking_lotas a default, but allow users to use those features without parking lot. The implementation details of this are sealed from the public API, so there are no breaking changes.I am open to suggestions regarding the implementation of this, specifically around handling mutex poisoning.