Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

remove the T: Sync requirement for RwLock<T>: Send #45267

Merged
merged 1 commit into from
Nov 1, 2017
Merged

remove the T: Sync requirement for RwLock<T>: Send #45267

merged 1 commit into from
Nov 1, 2017

Conversation

oconnor663
Copy link
Contributor

That requirement makes sense for containers like Arc that don't
uniquely own their contents, but RwLock is not one of those.

This restriction was added in 380d23b, but it's not clear why. @hniksic
and I were discussing this on reddit. I might be totally wrong about this change being sound, but I'm super curious to find out :)

@oconnor663
remove the T: Sync requirement for RwLock<T>: Send
fbf6885 
That requirement makes sense for containers like `Arc` that don't
uniquely own their contents, but `RwLock` is not one of those.

This restriction was added in
rust-lang@380d23b,
but it's not clear why.
@rust-highfive
Copy link
Collaborator

r? @aturon

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

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 14, 2017
@cuviper
Copy link
Member

cuviper commented Oct 16, 2017

RwLock<T> without Sync behaves basically like a RefCell<T>, whose Send also only requires T: Send.

@alexcrichton
Copy link
Member

Thanks for the PR! I think this change makes sense as well, especially along the lines of @cuviper's reasoning. I'm curious, though, what's the rationale for this change? Is code not compiling in the wild which should?

@oconnor663
Copy link
Contributor Author

This only came up for me because I was trying to make a table of the Send and Sync bounds that different "interior mutability" types take. I think looking at the precise API differences between Cell/RefCell/RwLock/Mutex is really helpful for illustrating how the safety rules work, and why they're designed the way they are. But yeah, I'd be kind of shocked if anyone ran into this in the wild.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 19, 2017
@alexcrichton
Copy link
Member

Hm ok, thanks for the info. In that sense I'd like to be extra careful with this, so to get some more eyes I'll....

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Oct 19, 2017 •
edited by dtolnay
Loading

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 19, 2017
@kennytm kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2017
@hniksic
Copy link
Contributor

hniksic commented Oct 20, 2017

@alexcrichton Regarding which specific code doesn't compile and should, that would be any code that tries to send a RwLock<T> whose T is Send but not Sync to another thread. Whether such a code is useful in practice is another question.

I would still argue for the change to be accepted, for the following reasons:

  • the current bound is stricter than it needs to be, which can lead to incorrect compiler diagnostics. The compiler disallows a perfectly safe construct (sending a RwLock<T> to another thread where T is Send but not Sync), inferring that it is unsound on basis of incorrect (too strict) trait bound on RwLock.
  • the bound is confusing people who are attempting to learn about Rust's threading model by inspecting the stdlib sources.
  • finally, this PR simply reverts the bound to the one originally written, and likely intended by the author of RwLock. The commit that introduced the current bound was about an unrelated issue and by the looks of it, the bound tightening was accidental.

@oconnor663
Copy link
Contributor Author

oconnor663 commented Oct 20, 2017 •
edited
Loading

For what it's worth, that commit also changed RwLock from being always Sync, to being Sync when the contents are Sync. That must've fixed a huge soundness hole? (Edit: @cuviper points out that I missed the impl bound.)

@cuviper
Copy link
Member

cuviper commented Oct 20, 2017

At the time, all of the methods were in impl<T: Send + Sync>, so I'm not sure there's anything unsound you could have done with RwLock being unconditionally Sync itself.

@aturon
Copy link
Member

aturon commented Oct 25, 2017

cc @RalfJung

@RalfJung
Copy link
Member

Indeed, RwLock: Send doesn't need the T: Sync bound. We have shown this in our formalization. I think I intended to submit a PR, but it seems I forgot to do so.

@RalfJung
Copy link
Member

RalfJung commented Oct 25, 2017 •
edited
Loading

Actually there are some more unnecessary bounds, but they are implicit: RwLockReadGuard<'a, T>: Sync only needs T: Sync, while Rust also demands T: Send. Similar for the write guard.

@oconnor663
Copy link
Contributor Author

That is really cool :-D Should I add those removals to this PR, or better to make a separate one?

@alexcrichton
Copy link
Member

@bors: r+

Thanks for taking a look @RalfJung!

@bors
Copy link
Contributor

bors commented Oct 31, 2017

📌 Commit fbf6885 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 1, 2017

⌛ Testing commit fbf6885 with merge 31bbe57...

bors added a commit that referenced this pull request Nov 1, 2017
@bors
Auto merge of #45267 - oconnor663:rwlock_send, r=alexcrichton
31bbe57 
remove the `T: Sync` requirement for `RwLock<T>: Send`

That requirement makes sense for containers like `Arc` that don't
uniquely own their contents, but `RwLock` is not one of those.

This restriction was added in 380d23b, but it's not clear why. @hniksic
and I [were discussing this on reddit](https://www.reddit.com/r/rust/comments/763o7r/blog_posts_introducing_lockfree_rust_comparing/dobcvbm/). I might be totally wrong about this change being sound, but I'm super curious to find out :)
@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 1, 2017
@bors
Copy link
Contributor

bors commented Nov 1, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 31bbe57 to master...

@bors bors merged commit fbf6885 into rust-lang:master Nov 1, 2017
@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 1, 2017
@oconnor663 oconnor663 deleted the rwlock_send branch November 1, 2017 14:56
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 2, 2017
@bstrie
Copy link
Contributor

bstrie commented Nov 7, 2017

@oconnor663 Feel free to make another PR removing the remaining bounds if you're up to it. :)

@RalfJung
Copy link
Member

RalfJung commented Nov 8, 2017

I already did: #45682

@dtolnay dtolnay mentioned this pull request Nov 18, 2017
Closed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees

@aturon aturon

Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.