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

Restore DISCONNECTED state in oneshot::Packet::send #36893

Merged
merged 1 commit into from
Oct 6, 2016
Merged

Restore DISCONNECTED state in oneshot::Packet::send #36893

merged 1 commit into from
Oct 6, 2016

Conversation

apasel422
Copy link
Contributor

Closes #32114

I'm not sure if this is the best approach, but the current action of swapping DISCONNECTED with DATA seems wrong. Additionally, it is strange that the send method (and others in the oneshot module) takes &mut self despite performing atomic operations, as this requires extra discipline to avoid data races and lets us use methods like AtomicUsize::get_mut instead of methods that require a memory ordering.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@alexcrichton
Copy link
Member

Thanks for the PR! I think this is almost everything we need here, but we may also want to alter the self.upgrade field to go back to NothingSent. Otherwise the second send will keep hammering on self.upgrade(up) which will successfully keep failing, but otherwise keeps allocating an upgrade channel and then keeps discarding it. Does that make sense?

Additionally, it is strange that the send method (and others in the oneshot module) takes &mut self despite performing atomic operations

Believe it or not, all of this code was written before &self vs &mut self had been settled in terms of access across threads! Today virtually all fields here should be UnsafeCell for sure.

@alexcrichton
Copy link
Member

I've opened up #36934 about the mutability concerns. I hasn't seemed to be a problem yet, but I'm sure as we start optimizing more aggressively it'll cause problems!

@alexcrichton alexcrichton assigned alexcrichton and unassigned brson Oct 3, 2016
@apasel422
Copy link
Contributor Author

@alexcrichton Makes sense. I'll try to make the modification later today.

@apasel422
Copy link
Contributor Author

@alexcrichton Done.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 5, 2016

📌 Commit fb90e4c has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 6, 2016

⌛ Testing commit fb90e4c with merge 46957f0...

bors added a commit that referenced this pull request Oct 6, 2016
Restore `DISCONNECTED` state in `oneshot::Packet::send`

Closes #32114

I'm not sure if this is the best approach, but the current action of swapping `DISCONNECTED` with `DATA` seems wrong. Additionally, it is strange that the `send` method (and others in the `oneshot` module) takes `&mut self` despite performing atomic operations, as this requires extra discipline to avoid data races and lets us use methods like `AtomicUsize::get_mut` instead of methods that require a memory ordering.
@bors bors merged commit fb90e4c into rust-lang:master Oct 6, 2016
@apasel422 apasel422 deleted the issue-32114 branch October 6, 2016 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants