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

mutex: add OwnedMutexGuard for Arc<Mutex<T>>s #2455

Merged
merged 5 commits into from
Apr 29, 2020
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Apr 28, 2020

This PR adds a new OwnedMutexGuard type and lock_owned and
try_lock_owned methods for Arc<Mutex<T>>. This is pretty much the
same as the similar APIs added in #2421.

I've also corrected some existing documentation that incorrectly
implied that the existing lock method cloned an internal Arc — I
think this may be a holdover from tokio 0.1's Lock type?

Signed-off-by: Eliza Weisman eliza@buoyant.io

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw added C-enhancement Category: A PR with an enhancement or bugfix. A-tokio Area: The main tokio crate M-sync Module: tokio/sync S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2020
@hawkw hawkw self-assigned this Apr 28, 2020
Comment on lines 124 to 129
/// As long as you have this guard, you have exclusive access to the underlying `T`. The guard
/// internally keeps a reference-couned pointer to the original `Mutex`, so even if the lock goes
/// away, the guard remains valid.
///
/// The lock is automatically released whenever the guard is dropped, at which point `lock`
/// will succeed yet again.
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 wrap these lines at 80 characters like the paragraph above them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do. There was some existing inconsistent wrapping in this file so I wasn't sure what style to follow.

Copy link
Contributor

Choose a reason for hiding this comment

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

The 80 characters ensure that the github view doesn't wrap lines in split mode, at least on my screen, so I like that width. It's also used relatively widely.

Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot this paragraph 😅

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 definitely also prefer 80 characters. :) I just also try to avoid changing the wrapping of existing comments that I haven't actually changed, so the git blame is clearer.

check_send::<MutexGuard<'_, u32>>();
check_send::<OwnedMutexGuard<u32>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider moving this to tests/async_send_sync.rs, which is not only able to detect missing Send impls, but also able to detect if something is Send when it shouldn't be. The file currently has mostly async functions, but it can handle types too.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
sorry alice

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member Author

hawkw commented Apr 29, 2020

MSRV test step on CI failed due to a network timeout while installing Rust, I restarted it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-sync Module: tokio/sync S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants