Add example of thinking about Send/Sync's soundness #259

dzfranklin · 2021-03-22T23:00:11Z

Add an example of thinking through whether it is sound to implement Send + Sync for a custom type that wraps a raw pointer.

I read the existing docs and was confused about whether I could implement Send + Sync for a type I wrote that wraps a c-style array. Kixiron, InfernoDeity, Talchas, and HeroicKatora on #black-magic helped me understand Send and Sync better. This example is based on the advice they gave me. I've made lots of changes, so any errors are probably mine.

Add an example of thinking through whether it is sound to implement Send + Sync for a custom type that wraps a raw pointer. I read the existing docs and was confused about whether I could implement Send + Sync for a type I wrote that wraps a c-style array. Kixiron, InfernoDeity, Talchas, and HeroicKatora on #black-magic helped me understand Send and Sync better. This example is based on the advice they gave me. I've made lots of changes, so any errors are probably mine.

Making the code executable would require adding a lot of duplicated hidden lines, plus libc isn't available.

dzfranklin · 2021-03-22T23:20:44Z

I think my example would be improved by an example of a change that would make Carton Send but not Sync and neither Send nor Sync, but I can't think of any simples ones that don't violate the safety invarients of casting pointers.

src/send-and-sync.md

JohnTitor

Thanks for writing up it! As a quick review, left some suggestions.

src/send-and-sync.md

danielhenrymantilla

Careful, there are 4 soundness issues in your Carton ⚠️

T may require an alignment bigger than the one calloc delivers;
mem::zeroed::<T> may not be a valid instance of type T, or it may even just not be a safe instance, so it should not be &mut-overwritten;
Missing Send bound on the typre parameter when implementing Send
Ditto for Sync

If this is gonna be a tutorial, then I recommend you feature a very nice trick to avoid these Send / Sync accidents 🙂: when writing a type that is, conceptually, like a stdlib type, then write an impl based on that:

  unsafe
  impl<T> Send for Carton<T>
+ where // when
+     Box<T> : Send,
  {}

src/send-and-sync.md

dzfranklin · 2021-03-22T23:54:42Z

Thank you everyone. I appreciate you all taking the time to point out my mistakes.

I have one question. I used rust,ignore because I got a build error that libc wasn't available and I'd otherwise a lot of hidden duplicate lines to make each example compile on its own. I saw a lot of other examples do the same. Is there a different solution I should do?

JohnTitor · 2021-03-23T00:15:15Z

I have one question. I used rust,ignore because I got a build error that libc wasn't available and I'd otherwise a lot of hidden duplicate lines to make each example compile on its own. I saw a lot of other examples do the same. Is there a different solution I should do?

I'd like to avoid using rust,ignore so that we can notice if it's stalled. And note that I don't put suggestions for the snippet that uses libc but for the snippets that don't have any external deps. Indeed, we have a bunch of rust,ignore snippets but I'm planning to re-evaluate and clean-up it once I get some time to it.

danielhenrymantilla pointed out the issues.

Co-authored-by: Yuki Okushi <huyuumi.dev@gmail.com>

danielhenrymantilla

Don't forget to impl Drop!

src/send-and-sync.md

danielhenrymantilla · 2021-03-23T21:06:48Z

src/send-and-sync.md

+Finally, lets think about whether our `Carton` is Send and Sync. Something can
+safely be Send unless it shares mutable state with something else without
+enforcing exclusive access to it. Each `Carton` has a unique pointer, so
+we're good.
+
+```rust
+# struct Carton<T>(std::ptr::NonNull<T>);
+// Safety: No one besides us has the raw pointer, so we can safely transfer the
+// Carton to another thread if T can be safely transferred.
+unsafe impl<T> Send for Carton<T> where T: Send {}


Right now Carton<T> does not implement Drop, so there is a memory leak. Once it does (with a call to libc::free(self.0.as_ptr().cast());, part of the requirements of Send would be that free itself can be called on a pointer that yielded by an allocation done on another thread. This is the case of free, so we may forget to mention it, but it's a requirement nonetheless.

A nice example where this does not happen is with a MutexGuard: notice how it is not Send. Indeed, the Rust implementation of mutexes make use of functions (`pthread_ that explicitly mention the handles they yield cannot be used from another thread, even if the access is unique.

See also this comment

That's a really good explanation. I tweaked it a little and added it to the page (a22d055).

src/send-and-sync.md

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

Based on [a comment][comment] by danielhenrymantilla [comment]: rust-lang#259 (comment)

src/send-and-sync.md

danielhenrymantilla · 2021-03-23T22:30:07Z

src/send-and-sync.md

+able to Send a MutexGuard to another thread the destructor would run in the
+thread you sent it to, violating the requirement. MutexGuard can still be Sync
+because all you can send to another thread is an `&MutexGuard` and dropping a
+reference does nothing.


Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

dzfranklin · 2021-03-25T18:12:32Z

I believe all the issues are addressed.

danielhenrymantilla

The main constructor example wasn't "cargo check-ed", and I think it shouldn't take too much effort to fix that (just mock libc by using a module, as suggested). Other than that, the code snippets are sound, and the comments seem appropriate: LGTM

src/send-and-sync.md

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

dzfranklin · 2021-03-29T23:27:08Z

I think this is ready @JohnTitor . Thank you everyone for helping me fix my code!

JohnTitor

Thanks! Just read and it looks great to me with some nits.

src/send-and-sync.md

Co-authored-by: Yuki Okushi <jtitor@2k36.org>

dzfranklin · 2021-04-01T12:43:07Z

Thanks for catching those issues. All fixed.

JohnTitor · 2021-04-01T12:59:03Z

Thanks again for the great work!

dzfranklin · 2021-04-01T12:59:58Z

Thank you for holding my hand so much! I've learned a lot.

Update books ## nomicon 1 commits in 6fe476943afd53a9a6e91f38a6ea7bb48811d8ff..8551afbb2ca6f5ea37fe58380318b209785e4e02 2021-03-10 07:28:57 +0900 to 2021-04-01 21:58:50 +0900 - Add example of thinking about Send/Sync's soundness (rust-lang/nomicon#259) ## reference 10 commits in fd97729e2d82f8b08d68a31c9bfdf0c37a7fd542..e1abb17cd94cd5a8a374b48e1bc8134a2208ed48 2021-03-28 14:29:19 -0700 to 2021-04-07 08:09:48 -0700 - Update introduction.md (rust-lang/reference#1004) - clarify UB for raw ptr deref (rust-lang/reference#1000) - Update lint level documentation. (rust-lang/reference#998) - Add rustdoc to tool lints. (rust-lang/reference#997) - Link to ptr::addr_of on raw pointer docs (rust-lang/reference#993) - apply rust-lang/reference#950 to STYLE.md (rust-lang/reference#980) - Tuple Passover rust-lang/reference#2 (rust-lang/reference#990) - Fix typo in macros-by-example.md (rust-lang/reference#996) - Clarify object safety rules for methods striked from the vtable (rust-lang/reference#965) - Add const generic args to const contexts. (rust-lang/reference#995) ## rust-by-example 1 commits in 29d91f591c90dd18fdca6d23f1a9caf9c139d0d7..c80f0b09fc15b9251825343be910c08531938ab2 2021-03-23 09:03:39 -0300 to 2021-04-08 10:28:17 -0300 - fix compile bug with panic! (rust-lang/rust-by-example#1433) ## rustc-dev-guide 11 commits in 0687daa..a9bd2bb 2021-03-28 13:33:56 -0400 to 2021-04-09 18:12:21 -0400 - Improve formatting and update info in "method lookup" section - Change wording a bit: `module` => `crate` - fix typo (rust-lang/rustc-dev-guide#1107) - fix typo - Mention CI build of LLVM in build instruction - Fix rustdocs test command typo (rust-lang/rustc-dev-guide#1103) - Update the "LLVM updates" section - Fix a link about Rustdoc internals - Add quickstart for adding a new optimization (rust-lang/rustc-dev-guide#1094) - Add back example of {{cwd}} (rust-lang/rustc-dev-guide#1099) - Document test input normalization ## embedded-book 1 commits in d3f2ace94d51610cf3e3c265705bb8416d37f8e4..569c3391f5c0cc43433bc77831d17f8ff4d76602 2021-03-17 07:53:09 +0000 to 2021-04-07 08:32:11 +0000 - Make it easier to copy and paste example commands. (rust-embedded/book#289)

dzfranklin added 2 commits March 22, 2021 22:58

Don't execute example code

8739b01

Making the code executable would require adding a lot of duplicated hidden lines, plus libc isn't available.

danielhenrymantilla reviewed Mar 22, 2021

View reviewed changes

src/send-and-sync.md Outdated Show resolved Hide resolved

JohnTitor requested a review from a team March 22, 2021 23:42

JohnTitor requested changes Mar 22, 2021

View reviewed changes

danielhenrymantilla suggested changes Mar 22, 2021

View reviewed changes

src/send-and-sync.md Outdated Show resolved Hide resolved

src/send-and-sync.md Outdated Show resolved Hide resolved

src/send-and-sync.md Outdated Show resolved Hide resolved

dzfranklin and others added 6 commits March 23, 2021 20:27

Fix soundness issues in new

c3ba178

danielhenrymantilla pointed out the issues.

Fix missing trait bounds

05a2df5

Reduce untested code

29f7139

Use american spelling

2dd3566

Co-authored-by: Yuki Okushi <huyuumi.dev@gmail.com>

Fix formatting

357eab4

Co-authored-by: Yuki Okushi <huyuumi.dev@gmail.com>

Add heading

e542c32

danielhenrymantilla reviewed Mar 23, 2021

View reviewed changes

dzfranklin and others added 3 commits March 23, 2021 21:14

Make out param stack local, fix for zero sized types

75253ec

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

Description fixes by danielhenrymantilla

53661a5

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

Qualify with ptr::

9f9f909

dzfranklin added a commit to dzfranklin/nomicon that referenced this pull request Mar 23, 2021

Fix memory leak and discuss implications

a22d055

Based on [a comment][comment] by danielhenrymantilla [comment]: rust-lang#259 (comment)

Fix memory leak and discuss implications

a5da003

Based on [a comment][comment] by danielhenrymantilla [comment]: rust-lang#259 (comment)

dzfranklin force-pushed the send-sync-example branch from a22d055 to a5da003 Compare March 23, 2021 21:54

Reword

d060a51

danielhenrymantilla reviewed Mar 23, 2021

View reviewed changes

src/send-and-sync.md Outdated Show resolved Hide resolved

danielhenrymantilla reviewed Mar 23, 2021

View reviewed changes

Update src/send-and-sync.md

09d4be2

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

danielhenrymantilla approved these changes Mar 25, 2021

View reviewed changes

src/send-and-sync.md Outdated Show resolved Hide resolved

src/send-and-sync.md Outdated Show resolved Hide resolved

dzfranklin and others added 2 commits March 25, 2021 20:06

Update src/send-and-sync.md

484bbeb

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

Update src/send-and-sync.md

d1c89a8

Co-authored-by: Daniel Henry-Mantilla <daniel.henry.mantilla@gmail.com>

Fix failing tests

7a1be9d

JohnTitor approved these changes Mar 31, 2021

View reviewed changes

src/send-and-sync.md Outdated Show resolved Hide resolved

src/send-and-sync.md Outdated Show resolved Hide resolved

src/send-and-sync.md Outdated Show resolved Hide resolved

src/send-and-sync.md Outdated Show resolved Hide resolved

dzfranklin and others added 4 commits April 1, 2021 13:42

Update src/send-and-sync.md

33fa3de

Co-authored-by: Yuki Okushi <jtitor@2k36.org>

Update src/send-and-sync.md

4a910d8

Co-authored-by: Yuki Okushi <jtitor@2k36.org>

Update src/send-and-sync.md

047dc6b

Co-authored-by: Yuki Okushi <jtitor@2k36.org>

Update src/send-and-sync.md

6495701

Co-authored-by: Yuki Okushi <jtitor@2k36.org>

JohnTitor merged commit 8551afb into rust-lang:master Apr 1, 2021

ehuss mentioned this pull request Apr 14, 2021

Update books rust-lang/rust#84191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add example of thinking about Send/Sync's soundness #259

Add example of thinking about Send/Sync's soundness #259

dzfranklin commented Mar 22, 2021

dzfranklin commented Mar 22, 2021 •

edited

Loading

JohnTitor left a comment

danielhenrymantilla left a comment •

edited

Loading

dzfranklin commented Mar 22, 2021

JohnTitor commented Mar 23, 2021

danielhenrymantilla left a comment

danielhenrymantilla Mar 23, 2021

dzfranklin Mar 23, 2021 •

edited

Loading

danielhenrymantilla Mar 23, 2021

dzfranklin commented Mar 25, 2021

danielhenrymantilla left a comment

dzfranklin commented Mar 29, 2021 •

edited

Loading

JohnTitor left a comment

dzfranklin commented Apr 1, 2021

JohnTitor commented Apr 1, 2021

dzfranklin commented Apr 1, 2021

Add example of thinking about Send/Sync's soundness #259

Add example of thinking about Send/Sync's soundness #259

Conversation

dzfranklin commented Mar 22, 2021

dzfranklin commented Mar 22, 2021 • edited Loading

JohnTitor left a comment

Choose a reason for hiding this comment

danielhenrymantilla left a comment • edited Loading

Choose a reason for hiding this comment

dzfranklin commented Mar 22, 2021

JohnTitor commented Mar 23, 2021

danielhenrymantilla left a comment

Choose a reason for hiding this comment

danielhenrymantilla Mar 23, 2021

Choose a reason for hiding this comment

dzfranklin Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

danielhenrymantilla Mar 23, 2021

Choose a reason for hiding this comment

dzfranklin commented Mar 25, 2021

danielhenrymantilla left a comment

Choose a reason for hiding this comment

dzfranklin commented Mar 29, 2021 • edited Loading

JohnTitor left a comment

Choose a reason for hiding this comment

dzfranklin commented Apr 1, 2021

JohnTitor commented Apr 1, 2021

dzfranklin commented Apr 1, 2021

dzfranklin commented Mar 22, 2021 •

edited

Loading

danielhenrymantilla left a comment •

edited

Loading

dzfranklin Mar 23, 2021 •

edited

Loading

dzfranklin commented Mar 29, 2021 •

edited

Loading