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

MutexGuard<T> may be Sync only if T is Sync #41624

Merged
merged 2 commits into from May 3, 2017

Conversation

Projects
None yet
@RalfJung
Member

RalfJung commented Apr 29, 2017

Fixes #41622

This is a breaking change. Does that imply any further process?

I am not sure whether I wrote that "compilation must fail"-test correctly, but at least it is passing here with the patch applied. Same for the since = "1.18.0", I just picked it because I had to pick something.

MutexGuard<T> may be Sync only if T is Sync
Also remove some unnecessary unsafe impl from the tests.
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Apr 29, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Collaborator

rust-highfive commented Apr 29, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Apr 29, 2017

Member

Travis complains about the since value changing. I thought I'd have to change it because this is really a completely different impl than the old one.
Should I change the since to say 1.0.0?

Member

RalfJung commented Apr 29, 2017

Travis complains about the since value changing. I thought I'd have to change it because this is really a completely different impl than the old one.
Should I change the since to say 1.0.0?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 29, 2017

Member

Thanks for catching this @RalfJung! I'm personally ahead going ahead and landing this regardless of the breakage. It's (a) a soundness fix and (b) seems unlikely to be relied upon in practice. We've got a lot of runway before this reaches stable to detect regressions and help upstream authors fix the issue. In any case cc @rust-lang/libs for the breakage aspect.

For the tidy error here yeah you can just pick a new feature name that's not "rust1", and it can be something arbitrary.

Member

alexcrichton commented Apr 29, 2017

Thanks for catching this @RalfJung! I'm personally ahead going ahead and landing this regardless of the breakage. It's (a) a soundness fix and (b) seems unlikely to be relied upon in practice. We've got a lot of runway before this reaches stable to detect regressions and help upstream authors fix the issue. In any case cc @rust-lang/libs for the breakage aspect.

For the tidy error here yeah you can just pick a new feature name that's not "rust1", and it can be something arbitrary.

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Apr 29, 2017

Member

All right, I picked a new feature name (mutexguard) and pushed that as a new commit here.

Member

RalfJung commented Apr 29, 2017

All right, I picked a new feature name (mutexguard) and pushed that as a new commit here.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 May 2, 2017

Contributor

@alexcrichton - I think this is ready for merge. Friendly ping to keep this on your radar.

Contributor

arielb1 commented May 2, 2017

@alexcrichton - I think this is ready for merge. Friendly ping to keep this on your radar.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 2, 2017

Member

Ok it's been a few days so I'm going to r+, but we may still back out and determine a new strategy to land if we discover this causes a number of regressions on crater. Regardless we'll get this in somehow!

@bors: r+

Member

alexcrichton commented May 2, 2017

Ok it's been a few days so I'm going to r+, but we may still back out and determine a new strategy to land if we discover this causes a number of regressions on crater. Regardless we'll get this in somehow!

@bors: r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 2, 2017

Contributor

📌 Commit 23522f6 has been approved by alexcrichton

Contributor

bors commented May 2, 2017

📌 Commit 23522f6 has been approved by alexcrichton

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 2, 2017

Contributor

Ordinarily, we would do a crater run first. But I agree this seems like quite the corner case.

Contributor

nikomatsakis commented May 2, 2017

Ordinarily, we would do a crater run first. But I agree this seems like quite the corner case.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 3, 2017

Contributor

⌛️ Testing commit 23522f6 with merge 0634f0a...

Contributor

bors commented May 3, 2017

⌛️ Testing commit 23522f6 with merge 0634f0a...

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

Auto merge of #41624 - RalfJung:mutexguard-sync, r=alexcrichton
MutexGuard<T> may be Sync only if T is Sync

Fixes #41622

This is a breaking change. Does that imply any further process?

I am not sure whether I wrote that "compilation must fail"-test correctly, but at least it is passing here with the patch applied. Same for the `since = "1.18.0"`, I just picked it because I had to pick something.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 3, 2017

Contributor

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

Contributor

bors commented May 3, 2017

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

@bors bors merged commit 23522f6 into rust-lang:master May 3, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@pmarcelll

This comment has been minimized.

Show comment
Hide comment
@pmarcelll

pmarcelll May 3, 2017

Contributor

I think since = "1.18.0" should be since = "1.19.0" (1.18 is already in beta).

Contributor

pmarcelll commented May 3, 2017

I think since = "1.18.0" should be since = "1.19.0" (1.18 is already in beta).

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Jun 9, 2017

Member

That's right, this will only land with 1.19. Is that a problem?

Member

RalfJung commented Jun 9, 2017

That's right, this will only land with 1.19. Is that a problem?

@bluss

This comment has been minimized.

Show comment
Hide comment
@bluss

bluss Jun 9, 2017

Contributor

it's fixed in master, so nightly docs show the correct Rust 1.19

Contributor

bluss commented Jun 9, 2017

it's fixed in master, so nightly docs show the correct Rust 1.19

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Jun 9, 2017

Member

Ah, cool. Actually, it's fixed even in the beta: https://doc.rust-lang.org/beta/std/sync/struct.MutexGuard.html

Member

RalfJung commented Jun 9, 2017

Ah, cool. Actually, it's fixed even in the beta: https://doc.rust-lang.org/beta/std/sync/struct.MutexGuard.html

@ubsan

This comment has been minimized.

Show comment
Hide comment
@ubsan

ubsan Jun 10, 2017

Contributor

hmm. I disagree with this fix. MutexGuard should just own a PhantomData<&mut T>... that would fix all the problems with it, and would be more correct with other auto traits. I wish I had seen this sooner.

Contributor

ubsan commented Jun 10, 2017

hmm. I disagree with this fix. MutexGuard should just own a PhantomData<&mut T>... that would fix all the problems with it, and would be more correct with other auto traits. I wish I had seen this sooner.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Jun 10, 2017

Member

So &Mutex<T> is not enough because T: Send + !Sync is allowed in a Mutex, AFAICT.

But, theoretically, someone could write an unsafe abstraction that isn't thread-safe in a Mutex, and they might remove Sync but keep Send because passing it around as an owned value works fine.

The thought of that is rather unsettling, and one more thing to keep in mind about unsafe code.
But I am hopeful @RalfJung's and others' work on unsafe correctness will keep us safe in the future.

Member

eddyb commented Jun 10, 2017

So &Mutex<T> is not enough because T: Send + !Sync is allowed in a Mutex, AFAICT.

But, theoretically, someone could write an unsafe abstraction that isn't thread-safe in a Mutex, and they might remove Sync but keep Send because passing it around as an owned value works fine.

The thought of that is rather unsettling, and one more thing to keep in mind about unsafe code.
But I am hopeful @RalfJung's and others' work on unsafe correctness will keep us safe in the future.

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Jun 10, 2017

Member

hmm. I disagree with this fix. MutexGuard should just own a PhantomData<&mut T>... that would fix all the problems with it, and would be more correct with other auto traits. I wish I had seen this sooner.

If you mean the fix is to add that field and leave the existing fields unchanged, I disagree. That would restrict MutexGuard<T> to be Sync only when T is both Send and Sync, which is unnecessarily restrictive.
That said, a Mutex with a non-Send T seems like a rather pointless exercise, so maybe this doesn't matter.

So &Mutex is not enough because T: Send + !Sync is allowed in a Mutex, AFAICT.

That's right. Mutex<T> is Send + Sync when T is Send. No Sync-bound on T needed.

Member

RalfJung commented Jun 10, 2017

hmm. I disagree with this fix. MutexGuard should just own a PhantomData<&mut T>... that would fix all the problems with it, and would be more correct with other auto traits. I wish I had seen this sooner.

If you mean the fix is to add that field and leave the existing fields unchanged, I disagree. That would restrict MutexGuard<T> to be Sync only when T is both Send and Sync, which is unnecessarily restrictive.
That said, a Mutex with a non-Send T seems like a rather pointless exercise, so maybe this doesn't matter.

So &Mutex is not enough because T: Send + !Sync is allowed in a Mutex, AFAICT.

That's right. Mutex<T> is Send + Sync when T is Send. No Sync-bound on T needed.

@ubsan

This comment has been minimized.

Show comment
Hide comment
@ubsan

ubsan Jun 11, 2017

Contributor

@RalfJung that is a bit weird. It does feel as if MutexGuard<'a, T> should be treated exactly like an &mut T wrt auto trait impls... :/

Contributor

ubsan commented Jun 11, 2017

@RalfJung that is a bit weird. It does feel as if MutexGuard<'a, T> should be treated exactly like an &mut T wrt auto trait impls... :/

@RalfJung

This comment has been minimized.

Show comment
Hide comment
@RalfJung

RalfJung Jun 11, 2017

Member

Yes, and I agree with that. However, MutexGuard<T> also has an &Mutex<T> field, which the auto traits also take into account, and which further restricts them.

Member

RalfJung commented Jun 11, 2017

Yes, and I agree with that. However, MutexGuard<T> also has an &Mutex<T> field, which the auto traits also take into account, and which further restricts them.

@ubsan

This comment has been minimized.

Show comment
Hide comment
@ubsan

ubsan Jun 11, 2017

Contributor

@RalfJung right, that's what I'm thinking - this seems like a design flaw with auto traits :/ you can't have a field which isn't taken into account by auto traits.

Contributor

ubsan commented Jun 11, 2017

@RalfJung right, that's what I'm thinking - this seems like a design flaw with auto traits :/ you can't have a field which isn't taken into account by auto traits.

@RalfJung RalfJung deleted the RalfJung:mutexguard-sync branch Jul 15, 2017

@rkjnsn

This comment has been minimized.

Show comment
Hide comment
@rkjnsn

rkjnsn Jul 21, 2017

Contributor

Would it make sense for MutexGuard<T> to have both PhantomData<&mut T> and the explicit implementation of Sync? The explicit Sync implementation would mean it's not overly restrictive, while the PhantomData would hopefully make sure that MutexGuard doesn't end up improperly implementing any future unsafe auto traits that may be created?

Contributor

rkjnsn commented Jul 21, 2017

Would it make sense for MutexGuard<T> to have both PhantomData<&mut T> and the explicit implementation of Sync? The explicit Sync implementation would mean it's not overly restrictive, while the PhantomData would hopefully make sure that MutexGuard doesn't end up improperly implementing any future unsafe auto traits that may be created?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment