Add Arc::{incr,decr}_strong_count #70733
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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. |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Any examples where proposed implementation of |
|
@tmiasko I only added that method for the purpose of symmetry; if we're able to increment it seems desirable to also be able to decrement. But perhaps you're right and it doesn't serve a purpose. |
@Mark-Simulacrum I'm not quite sure I follow what you mean? Are you referring to changing the external API, the internal implementation, or both? @tmiasko I've gone and removed the |
yoshuawuyts
changed the title
I'm saying that I think the more ergonomic API is probably something like These functions would internally do the |
|
@Mark-Simulacrum Ahh, now I see. Yes, I agree fully. Going to go ahead and change things to match your suggestion (: |
|
I'd just like to say I'm excited for this feature because it will make writing certain kinds of unsafe code much easier. :) Bumping reference counts using Example: https://twitter.com/stjepang/status/1217906121614286848 |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Okay, did another pass. The two methods are now:
As per @Mark-Simulacrum's suggestion they take a pointer, which has improved practical use. NamingSomething I'm unsure about is the naming. There are several considerations:
I'm wondering whether |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
yoshuawuyts
changed the title
yoshuawuyts
changed the title
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
sfackler
reviewed
Apr 5, 2020
Matthias247
suggested changes
Apr 5, 2020
There was a problem hiding this comment.
The functions all seem to lead to wrong reference counts at the moment.
I highly recommend adding tests which check for the expected reference counts after invoking the functions.
I am surprised that no tests on the RawWaker are failing - are there none? I know the initial implementation I did during RawWaker stabilization had ones - but wasn't merged back then. Not sure about the test coverage of the recently added implementation. If there are none it would be good to check refcounts there too.
I agree that this API is more helpful for the use-case this is currently targetting. But I'm undecided whether I like it. It has not a lot of type information regarding what it targets ( |
|
@Matthias247 Note that these will always be called like |
current APIThis is the API we currently have to manipulate the strong reference count with // Increment the strong reference count
let waker: Arc<W> = Arc::from_raw(waker as *const W);
mem::forget(Arc::clone(&waker));
// Decrement the strong reference count
mem::drop(Arc::from_raw(waker as *const W));associated pointer APIThis is the (unsafe) API @Mark-Simulacrum proposed. This takes a pointer to an Arc, and increments / decrements the strong refcount. An upside of this API is that it works well for the intended use of the API (e.g. if you're managing an Arc pointer yourself). A downside is that unlike any other methods on Arc it takes a pointer. // Increment the strong reference count
Arc::incr_strong_count(waker as *const W);
// Decrement the strong reference count
Arc::decr_strong_count(waker as *const W);method APIAn alternative would be to provide increment/decrement methods on functions that consume an // Increment the strong reference count
let arc = Arc::from_raw(waker as *const W);
arc.incr_strong_count();
// Decrement the strong reference count
let arc = Arc::from_raw(waker as *const W);
arc.decr_strong_count();This is closer to the design I originally proposed. But despite it being more consistent with existing methods, I think the associated pointer design is superior. The only reasonable case I can think of when counters should be manipulated manually is when manually using Arc through a pointer. One added benefit that comes with the associated pointer API is that it allows us to make progress on documenting using |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Matthias247
reviewed
Apr 5, 2020
That one won't really help, because the challenging part seems to be getting an I think the most misuse resistant API seems to be the one that @Mark-Simulacrum proposed. Given this discussion, I'm however not convinced that something like this should be added at all. It's kind of a hack, to make certain use-cases work without requiring to also write a custom |
@Matthias247 This issue was motivated by the use in #68700 (comment). And as @stjepang pointed out in #70733 (comment) even Rust experts regularly get this wrong. I agree this is not an API that's likely to be used by people new to Rust. But using Arc to construct a custom reference-counted structure is a legit use case, and one I think should be encouraged. Writing a custom reference counter using atomics is even harder to get right, increasing the likelihood bugs and vulnerabilities might occur. If we can make it easier for Arc to be used for custom reference counters I think that'd be a net gain. |
|
The methods seem to be fine, but I believe are needlessly noisy; when using this API you essentially always already have the raw pointer and nothing else. I'm not sure I follow when you say that the Arc-taking methods are misusable, since I think they're guaranteed to work (and indeed both would even be safe, though Arc::from_raw would be unsafe of course). I agree, though, that I believe the raw pointer taking methods are the best choice here, as I think they're easiest to understand. I pretty strongly disagree that std shouldn't provide them, though. I don't really see how it's a hack, rather it seems like a natural thing to want in unsafe code interacting with shared ownership (using Arc). Its also something that we know is being used - so providing a canonical way of doing so seems good. |
|
@bors r+ Thanks! |
|
📌 Commit b04599f has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit b04599f with merge 86487ffa5824c4c3e89363d8b095199bf876f166... |
|
@bors retry yield |
|
⌛ Testing commit b04599f with merge 59a0fec547f3fc3832f4b1f6b1637c7718279c13... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry (included in rollup so yielding) |
bors
added
S-waiting-on-bors
|
⌛ Testing commit b04599f with merge a04e20496a7c19dfe64923e069e7de8c8da8f22e... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 8, 2020
Rollup of 5 pull requests Successful merges: - rust-lang#70733 (Add Arc::{incr,decr}_strong_count) - rust-lang#71598 (improve Drop documentation) - rust-lang#71783 (Detect errors caused by `async` block in 2015 edition) - rust-lang#71903 (reword "possible candidate" import suggestion) - rust-lang#71960 (Fix E0284 to not use incorrect wording) Failed merges: r? @ghost
ghost
mentioned this pull request
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds two
unsafemethods toArc:incr_strong_countanddecr_strong_count. A suggestion to add methods to change the strong count inArccame up in during review in #68700 (comment), and from asking a few people this seemed like generally useful to have.References: