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

Shared<T> does not forward Sync #31456

Closed
alexreg opened this issue Feb 6, 2016 · 10 comments
Closed

Shared<T> does not forward Sync #31456

alexreg opened this issue Feb 6, 2016 · 10 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexreg
Copy link
Contributor

alexreg commented Feb 6, 2016

While it's correct for it not to forward the Send trait, because it could violate mutability rules, there seems to be no reason it shouldn't forward the Sync trait (as Unique<T> does).

@apasel422
Copy link
Contributor

I'm not sure that the definition of Sync would permit this, as you can get *mut Ts out of a &Shared<T>. From the docs:

The precise definition is: a type T is Sync if &T is thread-safe. In other words, there is no possibility of data races when passing &T references between threads.

The Nomicon has more details.

@alexreg
Copy link
Contributor Author

alexreg commented Feb 6, 2016

That is an unsafe operation however, so the usual rules don’t apply.

On 6 Feb 2016, at 22:21, Andrew Paseltiner notifications@github.com wrote:

I'm not sure that the definition of Sync would permit this, as you can get *mut Ts out of a &Shared. From the docs https://doc.rust-lang.org/std/marker/trait.Sync.html:

The precise definition is: a type T is Sync if &T is thread-safe. In other words, there is no possibility of data races when passing &T references between threads.

The Nomicon https://doc.rust-lang.org/nomicon/send-and-sync.html has more details.


Reply to this email directly or view it on GitHub #31456 (comment).

@bluss
Copy link
Member

bluss commented Feb 6, 2016

Since you can't access T from a Shared<T> without unsafe code, it doesn't matter for correctness either way. How Shared<T> behaves is just chosen for what's convenient in the places where it is used as a building block.

Edit to clarify: I see no reason to forward Sync.

@apasel422
Copy link
Contributor

@alexreg In that case, why is *mut T !Sync?

@Kimundi
Copy link
Member

Kimundi commented Feb 6, 2016

From the nomicon link you posted: "Doing anything useful with a raw pointer requires dereferencing it, which is already unsafe. In that sense, one could argue that it would be "fine" for them to be marked as thread safe." ;)

@alexreg
Copy link
Contributor Author

alexreg commented Feb 7, 2016

Anyway, if we’re going to use the argument “it can only be accessed unsafely”, then I think Unique<T> should not forward Send/Sync either, for the sake of consistency.

On 6 Feb 2016, at 23:22, Marvin Löbel notifications@github.com wrote:

From the nomicon link you posted: "Doing anything useful with a raw pointer requires dereferencing it, which is already unsafe. In that sense, one could argue that it would be "fine" for them to be marked as thread safe." ;)


Reply to this email directly or view it on GitHub #31456 (comment).

@nagisa
Copy link
Member

nagisa commented Feb 7, 2016

Anyway, if we’re going to use the argument “it can only be accessed unsafely”, then I think Unique<T> should not forward Send/Sync either, for the sake of consistency.

Documentation for Unique<T> explains the reasoning:

Unique pointers are Sync if T is Sync because the data they reference is unaliased. Note that this aliasing invariant is unenforced by the type system; the abstraction using the Unique must enforce it.

Shared has no such semantic attached to it.

@alexreg
Copy link
Contributor Author

alexreg commented Feb 7, 2016

This rather misses the point, since you can alias raw pointers anyway.

I think it’s all a big lie to be talking about guarantees with unsafe pointers anyway. Why not make this explicit by not pretending there are by implementing these traits?

On 7 Feb 2016, at 14:55, Simonas Kazlauskas notifications@github.com wrote:

Anyway, if we’re going to use the argument “it can only be accessed unsafely”, then I think Unique should not forward Send/Sync either, for the sake of consistency.

Documentation for Unique explains the reasoning:

Unique pointers are Sync if T is Sync because the data they reference is unaliased. Note that this aliasing invariant is unenforced by the type system; the abstraction using the Unique must enforce it.

Shared has no such semantic attached to it.


Reply to this email directly or view it on GitHub #31456 (comment).

@bluss
Copy link
Member

bluss commented Feb 7, 2016

Unique<T> and Shared<T> are conveniences, utilities, that encapsulate some recurring patterns in implementing Rc, Arc, and various collections. It's not about guarantees embodied by the Unique/Shared themselves, but defaults for properties w.r.t T that the type that uses Unique<T>/Shared<T> will want to have.

Being utility building blocks, it only makes sense to change them if you show why changed properties would be a better for their use case (current uses in libstd as well as cases in non-libstd code). For example, if a property of either Unique or Shared would make it easy to create bugs.

Note that Arc<T> has the following properties:

  • Arc<T>: Send where T: Send + Sync
  • Arc<T>: Sync where T: Send + Sync

so the suggested change is a correct default for neither Rc nor Arc.

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum
Copy link
Member

Based on #27730 (tracking issue) and #41064 (comment), we explicitly do not want to implement Sync for Shared<T>, so closing. If you disagree, please let us know and we'll reopen; also please leave a comment on the tracking issue voicing your opinion (we're headed towards FCP and stabilization of Shared, I believe, soon-ish).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants