Improve docs on Arc<T> and Send/Sync #41536

steveklabnik · 2017-04-25T15:25:01Z

This is something I always forget, so let's actually
explain in the docs.

I didn't fully link up everything here, but I'd like to make sure that the wording is okay before I bother.

rust-highfive · 2017-04-25T15:25:13Z

(rust_highfive has picked a reviewer for you, use r? to override)

oli-obk · 2017-04-25T15:32:15Z

src/liballoc/arc.rs

+/// `Arc<T>` will implement [`Send`] and [`Sync`] as long as the `T` implements
+/// [`Send`] and [`Sync`]. This may be a bit counter-intuitive at first: after
+/// all, isn't the point of `Arc<T>` thread safety? The key is this: `Arc<T>`
+/// is an implementation of multiple ownership that is thread safe, but it


Maybe state here, that Arc<T> is threadsafe in the way that you can safely clone or drop Arc<T>s pointing to the same data in multiple threads at the same time.

How would you construct this if you cannot send an arc to a different thread?

If you had a type T that is !Sync and an Arc<T> that does not give you access to the internal object, then the Arc<T> can be Send, because you can never access the inner data, but you can clone and drop the Arc<T> to your heart's desire. This is the guarantee that Arc<T> gives, without looking at the T, you can clone and drop the Arc<T>. Since Arc<T> also gives you access to it's inner data through Deref, you also need the Sync bound on T to make Arc<T> Send, but that's nothing special about Arc<T> but a general thing about sharing data over thread boundaries as stated in the Sync docs.

but impl<T> Send for Arc<T> where T: Send + Sync + ?Sized, that is, if T is !Sync, the Arc<T> will not be Send.

I understand that. I just meant to explain the reasoning behind that a bit in different words than "is an implementation of multiple ownership that is thread safe", because i totally stumbled over that sentence and would have not understood it at all 5 years ago

ah okay. I wish I had a way to re-word it that didn't involve something that's not true....... hm

Maybe something like

Arc<T> makes it thread safe to create multiple pointers to the same data, but it ...

I've tweaked this slightly, let me know what you think.

kennytm · 2017-04-25T23:16:42Z

src/liballoc/arc.rs

 /// more flexibility.
 ///
+/// `Arc<T>` will implement [`Send`] and [`Sync`] as long as the `T` implements
+/// [`Send`] and [`Sync`]. This may be a bit counter-intuitive at first: after
+/// all, isn't the point of `Arc<T>` thread safety? The key is this: `Arc<T>`


I find that saying "T: Send/Sync ⇔ Arc<T>: Send/Sync is counter-intuitive" is counter-intuitive 😅. It may be clearer to add one more sentence explaining the counter-intuitiveness, e.g. "Why it is not able hide the non-thread-safe type T to make itself Arc<T> thread-safe?"

killercup · 2017-04-26T08:45:01Z

src/liballoc/arc.rs

+///
+/// Unlike [`Rc<T>`], `Arc<T>` uses atomic operations for its reference
+/// counting.  This means that it is thread-safe. The disadvantage is that
+/// atomic operations are more expensive than ordinary memory accesses.  If you


The most tiniest of nits: What's with the double spaces after some sentences? 😄

steveklabnik · 2017-04-26T08:53:34Z

tl;dr: a bad vim config, and me missing fixes in some spots. ha!

…

On Wed, Apr 26, 2017 at 4:45 AM, Pascal Hertleif ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/liballoc/arc.rs <#41536 (comment)>: > @@ -54,16 +54,32 @@ const MAX_REFCOUNT: usize = (isize::MAX) as usize; /// exception. If you need to mutate through an `Arc`, use [`Mutex`][mutex], /// [`RwLock`][rwlock], or one of the [`Atomic`][atomic] types. /// -/// `Arc` uses atomic operations for reference counting, so `Arc`s can be -/// sent between threads. In other words, `Arc<T>` implements [`Send`] -/// as long as `T` implements [`Send`] and [`Sync`][sync]. The disadvantage is -/// that atomic operations are more expensive than ordinary memory accesses. -/// If you are not sharing reference-counted values between threads, consider -/// using [`rc::Rc`][`Rc`] for lower overhead. [`Rc`] is a safe default, because -/// the compiler will catch any attempt to send an [`Rc`] between threads. -/// However, a library might choose `Arc` in order to give library consumers +/// ## Thread Safety +/// +/// Unlike [`Rc<T>`], `Arc<T>` uses atomic operations for its reference +/// counting. This means that it is thread-safe. The disadvantage is that +/// atomic operations are more expensive than ordinary memory accesses. If you The most tiniest of nits: What's with the double spaces after *some* sentences? 😄 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#41536 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABsivgM8i7Yyeu8w72_Vei1fBZm4CW7ks5rzwQOgaJpZM4NHp0M> .

steveklabnik · 2017-05-03T09:59:27Z

I plan on updating this PR early next week; sorry about the delay.

BurntSushi · 2017-05-09T00:23:56Z

src/liballoc/arc.rs

+/// `RefCell<T>` isn't [`Send`], and if `Arc<T>` was always [`Send`],
+/// `Arc<RefCell<T>>` would be as well. But then we'd have a problem:
+/// `RefCell<T>` is not thread safe; it keeps track of the borrowing count
+/// using non-atomic operations. It works the same way with [`Sync`].


RefCell<T> is Send when T is Send. :-) I think you could either swap RefCell<T> for Rc<T> and change the "borrowing count" to "reference count," or you could keep RefCell<T> and change Send to Sync (since RefCell<T> is never Sync) and then drop the last sentence.

BurntSushi · 2017-05-09T00:25:15Z

src/liballoc/arc.rs

+/// using non-atomic operations. It works the same way with [`Sync`].
+///
+/// In the end, this means that you may need to pair `Arc<T>` with some sort of
+/// `std::sync` type, usually `Mutex<T>`.


Yeah, this is an important sentence, and it probably flows better if you keep RefCell in the preceding paragraph.

BurntSushi · 2017-05-09T00:27:20Z

I like this @steveklabnik. :-) r=me once feedback is addressed.

This is something I always forget, so let's actually explain in the docs.

steveklabnik · 2017-05-09T16:50:28Z

I believe I've fixed up everything, but I'd like to hear what you all have to say before I use @BurntSushi 's r=me 😄

kennytm · 2017-05-09T17:00:45Z

LGTM 👍. There is still an instance of double-space but I don't think it is a big issue.

BurntSushi · 2017-05-09T17:21:02Z

LGTM.

steveklabnik · 2017-05-09T17:38:24Z

@bors: r=burntsushi rollup

bors · 2017-05-09T17:38:25Z

📌 Commit 2f6744c has been approved by burntsushi

…ushi Improve docs on Arc<T> and Send/Sync This is something I always forget, so let's actually explain in the docs. I didn't fully link up everything here, but I'd like to make sure that the wording is okay before I bother.

Rollup of 5 pull requests - Successful merges: #41531, #41536, #41809, #41854, #41886 - Failed merges:

rust-highfive assigned BurntSushi Apr 25, 2017

oli-obk reviewed Apr 25, 2017

View reviewed changes

kennytm reviewed Apr 25, 2017

View reviewed changes

killercup reviewed Apr 26, 2017

View reviewed changes

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 26, 2017

BurntSushi reviewed May 9, 2017

View reviewed changes

Improve docs on Arc<T> and Send/Sync

2f6744c

This is something I always forget, so let's actually explain in the docs.

steveklabnik force-pushed the arc-and-send branch from 61c0d72 to 2f6744c Compare May 9, 2017 16:50

frewsxcv mentioned this pull request May 10, 2017

Rollup of 11 pull requests #41869

Closed

frewsxcv mentioned this pull request May 10, 2017

Rollup of 9 pull requests #41872

Closed

frewsxcv mentioned this pull request May 10, 2017

Rollup of 8 pull requests #41881

Closed

steveklabnik mentioned this pull request May 10, 2017

Rollup of 5 pull requests #41887

Merged

bors added a commit that referenced this pull request May 10, 2017

Auto merge of #41887 - steveklabnik:rollup, r=steveklabnik

978d2cf

Rollup of 5 pull requests - Successful merges: #41531, #41536, #41809, #41854, #41886 - Failed merges:

bors merged commit 2f6744c into rust-lang:master May 10, 2017

steveklabnik deleted the arc-and-send branch October 25, 2017 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve docs on Arc<T> and Send/Sync #41536

Improve docs on Arc<T> and Send/Sync #41536

steveklabnik commented Apr 25, 2017

rust-highfive commented Apr 25, 2017

oli-obk Apr 25, 2017 •

edited

Loading

steveklabnik Apr 26, 2017

oli-obk Apr 26, 2017

steveklabnik Apr 26, 2017

oli-obk Apr 26, 2017

steveklabnik Apr 27, 2017

oli-obk Apr 27, 2017

steveklabnik May 9, 2017

kennytm Apr 25, 2017

killercup Apr 26, 2017

steveklabnik commented Apr 26, 2017 via email

steveklabnik commented May 3, 2017

BurntSushi May 9, 2017

BurntSushi May 9, 2017

BurntSushi commented May 9, 2017

steveklabnik commented May 9, 2017

kennytm commented May 9, 2017

BurntSushi commented May 9, 2017

steveklabnik commented May 9, 2017

bors commented May 9, 2017

Improve docs on Arc<T> and Send/Sync #41536

Improve docs on Arc<T> and Send/Sync #41536

Conversation

steveklabnik commented Apr 25, 2017

rust-highfive commented Apr 25, 2017

oli-obk Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Apr 26, 2017 via email

steveklabnik commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurntSushi commented May 9, 2017

steveklabnik commented May 9, 2017

kennytm commented May 9, 2017

BurntSushi commented May 9, 2017

steveklabnik commented May 9, 2017

bors commented May 9, 2017

oli-obk Apr 25, 2017 •

edited

Loading