-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add a note comparing against std types #58
Conversation
Signed-off-by: John Nunley <[email protected]>
For Mutex, the lock fast path is a single CAS. event-listener used in slow path uses std mutex internally, but it would be different from "just use Lines 358 to 360 in 604d461
Lines 160 to 166 in 604d461
|
Signed-off-by: John Nunley <[email protected]>
Good point, I've clarified this by saying that it's part of the notification mechanism. |
src/lib.rs
Outdated
//! | ||
//! ## Relationship with `std::sync` | ||
//! | ||
//! In general, you should prefer to use [`std::sync`] types over types from this crate. |
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.
you should
Personally, I do not believe that the benefits of using std::sync or parking_lot here necessarily exceed the risk of accidentally holding the guard across the await point.
Especially when using parking_lot, it is easy to make that mistake because of the send guard. (I recently saw a colleague make that mistake and spend hours debugging.)
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.
Personally, I do not believe that the benefits of using std::sync or parking_lot here necessarily exceed the risk of accidentally holding the guard across the await point.
For standard library mutexes, there is a Clippy lint nowadays that prevents this scenario from happening. I've made the language less impactful, but I still think this isn't as large of an issue
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.
Maybe link to the Clippy lint? https://rust-lang.github.io/rust-clippy/stable/index.html#/await_holding_lock
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.
Good idea, I've added this
Co-authored-by: Jules Bertholet <[email protected]>
Signed-off-by: John Nunley <[email protected]>
Signed-off-by: John Nunley <[email protected]>
src/lib.rs
Outdated
//! When using [`std::sync`], you should be careful to not hold any locks over an `.await` point. | ||
//! In modern Rust, there is a Clippy lint called [`await_holding_lock`] that emits warnings for this | ||
//! scenario for [`std::sync`] and some other synchronization crates. Still, when deciding to use | ||
//! [`std::sync`] over `async-lock`, you must be careful to not hold any locks past an `.await` point. |
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.
This is quite repetitive, I think we can have something much shorter. For example, you could extend the sentence on line 19 to If you already use libstd and you aren't holding locks across await points (there is a [Clippy lint](link) to check for this), you should consider std::sync instead of this crate.
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.
Amended this
|
Co-authored-by: Jules Bertholet <[email protected]>
Signed-off-by: John Nunley <[email protected]>
Signed-off-by: John Nunley <[email protected]>
Closes #57