10

Stabilize Arc::{increment,decrement}_strong_count

 3 years ago
source link: https://github.com/rust-lang/rust/pull/79285
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.

Conversation

Member

yoshuawuyts commented on Nov 22

Tracking issue: #71983

Stabilizes Arc::{incr,decr}_strong_count, enabling unsafely incrementing an decrementing the Arc strong count directly with fewer gotchas. This API was first introduced on nightly six months ago, and has not seen any changes since. The initial PR showed two existing pieces of code that would benefit from this API, and included a change inside the stdlib to use this.

Given the small surface area, predictable use, and no changes since introduction, I'd like to propose we stabilize this.

r? @Mark-Simulacrum

Links

Member

Mark-Simulacrum commented on Nov 22

I am forgetting now -- is there a reason we didn't add this on Rc? I don't quickly see any discussion about that, but it seems like they'd be equally useful there ergonomics wise, though it's somewhat rarer (I suspect) to be stashing away Rc's in atomics or whatever where you're hiding them.

Member

Author

yoshuawuyts commented on Nov 22

edited

@Mark-Simulacrum I don't believe we discussed it, but there's no reason not to as far as I recall — I'm planning to add it to Rc and both Weak variants as a follow-up.

If there are no objections to stabilizing the Arc methods though, it'd be nice if we could stabilize that sooner.

Member

Mark-Simulacrum commented on Nov 23

This stabilizes the following methods on Arc, intended for manipulating the strong reference count without error-prone code required before (e.g., cloning the arc and forgetting it to increment or double-dropping an arc to decrement).

impl<T: ?Sized> Arc<T> {
    pub unsafe fn incr_strong_count(ptr: *const T);
    pub unsafe fn decr_strong_count(ptr: *const T);
}

The only reason these APIs are unsafe is that we take *const T rather than an Arc<T> or &Arc<T>; they are almost always used after Arc::into_raw, so this unsafety is deemed warranted. If we were to take an actual instance of Arc<T> the methods would be less useful as the caller would need to be careful to not re-drop the Arc in the case of &Arc<T> and/or not clone the Arc on passing it in in the owned case. Generally it's expected that this API is easier to use.

r? @dtolnay to kick off libs FCP on this.

Contributor

nagisa commented on Nov 23

Should incr and decr be expanded to a proper word before stabilizing?

Member

Author

yoshuawuyts commented 25 days ago

edited

@nagisa I find that increment_strong_count and decrement_strong_count feel rather verbose and would prefer we keep the naming as-is. The current names also have a nice benefit that they could also stand for "increase" and "decrease" which may make them easier to memorize.

Member

dtolnay commented 24 days ago

I am on board with these. #70733 has compelling discussion that this functionality is worth exposing in the standard library, and that these signatures involving *const T are the best ones.

@rfcbot fcp merge

rfcbot commented 24 days ago

edited

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

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

Member

BurntSushi commented 24 days ago

RE naming: I do have a mild preference for increment/decrement instead of abbreviating them. While these are useful, they are niche, so I don't really buy the verbosity argument. I don't think it's that important to reduce verbosity for infrequently used routines.

RE docs: Would it be possible to add some clarity to the docs explaining specifically why someone might want to use these routines?

Contributor

tmiasko commented 24 days ago

Personally I found those functions to be rather underwhelming. They cater to relatively niche use-cases, while having a trivial implementation. Furthermore, their own implementation uses a pattern which is much more generally applicable:

  • decrement: Arc::from_raw(ptr);
  • increment: let _ = ManuallyDrop::new(Arc::from_raw(ptr)).clone();

I would call neither of those trivial or easy to follow -- if I see those in the wild, I'm definitely going to expect a comment on them explaining them. I also don't think we guarantee that they will work (there's not really a way we could break that even if we wanted to and ignoring stability guarantees); I've always been rather uncertain about the "clone to increment" pattern externally from std, while internally we can of course use it.

Contributor

tmiasko commented 23 days ago

edited

I also don't think we guarantee that they will work (there's not really a way we could break that even if we wanted to and ignoring stability guarantees); I've always been rather uncertain about the "clone to increment" pattern externally from std, while internally we can of course use it.

I don't think there is anything contentious about Arc::from_raw or clone. The only part that could be controversial is using ManuallyDrop instead of Arc::into_raw. EDIT: Which might be a sign that a Arc::clone_raw might be more appropriate than Arc::incr_strong_count.

Member

m-ou-se commented 22 days ago

@rfcbot concern naming

See #79285 (comment).

I agree not abbreviating the names would be a bit better. Registering it as a concern so we don't forget.

Contributor

withoutboats commented 22 days ago

I struggled with the pointer aspect of this API a bit. It seemed strange to combine two steps - offseting to get an arc, and then manipulating the strong count - into one function. Especially so because incrementing the strong count is safe. I was worried that these were overly-tailored to implementing a waker.

What won me over to the current API was the drop problem: users who have a raw pointer have to make sure if they get an Arc from it that they know not to drop it. This is really tricky to get right because its all implicit; implementing it properly in std seems like the right choice.

Member

Author

yoshuawuyts commented 18 days ago

It seems everyone on the libs team has ticked their box! -- but the one outstanding concern is naming. So I'd like to ask: does anyone feel strongly against increment_strong_count, decrement_strong_count?

I've already expressed I prefer the short versions, but I don't feel strong enough about them to block this PR on that. So I want to check if anyone objects to the long-form instead. If not, I'll happily switch this PR over to use increment_strong_count, decrement_strong_count so it can be merged. Thanks!

yoshuawuyts

force-pushed the

yoshuawuyts:stabilize-arc_mutate_strong_count

branch from f66ec46 to b53d301

11 days ago

yoshuawuyts

changed the title Stabilize Arc::{incr,decr}_strong_count

Stabilize Arc::{increment,decrement}_strong_count

11 days ago

Member

Author

yoshuawuyts commented 11 days ago

I've update the PR to use the {increment,decrement}_strong_count method names. I believe once CI passes this should be ready for FCP.

Member

m-ou-se commented 11 days ago

@rfcbot resolve naming

rfcbot commented 11 days ago

bellThis is now entering its final comment period, as per the review above. bell

Collaborator

rust-log-analyzer commented 11 days ago

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ACTION=run6
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=yoshuawuyts
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_ca8f1ad1-b608-4e37-850c-af461e1f31bc
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=stabilize-arc_mutate_strong_count
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_ca8f1ad1-b608-4e37-850c-af461e1f31bc
GITHUB_REF=refs/pull/79285/merge
GITHUB_REPOSITORY_OWNER=rust-lang
GITHUB_RETENTION_DAYS=90
GITHUB_RUN_ID=431463215
GITHUB_RUN_NUMBER=21646
---
Collecting six>=1.5 (from python-dateutil<3.0.0,>=2.1; python_version >= "2.7"->botocore==1.12.197->awscli)
Building wheels for collected packages: PyYAML
  Running setup.py bdist_wheel for PyYAML: started
  Running setup.py bdist_wheel for PyYAML: finished with status 'error'
  Complete output from command /usr/bin/python3 -u -c "import setuptools, tokenize;__file__='/tmp/pip-build-an5_bj6z/PyYAML/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" bdist_wheel -d /tmp/tmpvx1izep2pip-wheel- --python-tag cp36:
     or: -c --help [cmd1 cmd2 ...]
     or: -c --help-commands
     or: -c cmd --help
  
---
    Checking cfg-if v0.1.10
    Checking adler v0.2.3
    Checking rustc-demangle v0.1.18
    Checking panic_abort v0.0.0 (/checkout/library/panic_abort)
error[E0599]: no function or associated item named `incr_strong_count` found for struct `Arc<_>` in the current scope
   --> library/alloc/src/task.rs:63:23
    |
63  |         unsafe { Arc::incr_strong_count(waker as *const W) };
    |                       |
    |                       |
    |                       function or associated item not found in `Arc<_>`
    |                       help: there is an associated function with a similar name: `increment_strong_count`
   ::: library/alloc/src/sync.rs:223:1
    |
    |
223 | pub struct Arc<T: ?Sized> {
    | ------------------------- function or associated item `incr_strong_count` not found for this

error[E0599]: no function or associated item named `decr_strong_count` found for struct `Arc<_>` in the current scope
   --> library/alloc/src/task.rs:84:23
    |
84  |         unsafe { Arc::decr_strong_count(waker as *const W) };
    |                       |
    |                       |
    |                       function or associated item not found in `Arc<_>`
    |                       help: there is an associated function with a similar name: `decrement_strong_count`
   ::: library/alloc/src/sync.rs:223:1
    |
    |
223 | pub struct Arc<T: ?Sized> {
    | ------------------------- function or associated item `decr_strong_count` not found for this
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0599`.
error: could not compile `alloc`
error: could not compile `alloc`

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:30
== clock drift check ==
  local time: Fri Dec 18 22:32:07 UTC 2020

yoshuawuyts

force-pushed the

yoshuawuyts:stabilize-arc_mutate_strong_count

branch from b53d301 to a55039d

11 days ago

Member

Author

yoshuawuyts commented 11 days ago

Oops, I forgot to update the usage of this API in task.rs -- fixed and pushed again!

Contributor

CAD97 commented 6 days ago

edited

FWIW, I use clone_raw(*const T) -> Arc<T> in my real-world implementation of "+0 Arc". It's currently implemented as the rather opaque let this = ManuallyDrop::new(Arc::from_raw(ptr)); Arc::clone(&this). This would allow me to implement it as Arc::increment_strong_count(ptr); Arc::from_raw(ptr), which I think is clearer. It also allows me to avoid the spec lawyer interpretation of from_raw "taking ownership" of a strong count and my never undoing that with the current impl; it would probably be more correctly written as (the sadly more error prone (due to holding the Arc by-value temporarily)) let this = Arc::from_raw(ptr); let clone = Arc::clone(&this); Arc::into_raw(this); clone, such that the strong count is "freed" back into the pool for raw pointers.

TL;DR I'm a +1 for stabilizing at least the increment functionality, and have another real-world potential usage. Increment has potential pitfalls in implementation.

The same crate contains a use for the weak count version of the same, as well.

(Though I might've just done clone_raw(*const T) -> Arc<T>, as the domain there is more clearly on the pointer, and increment_strong_count(_) = into_raw(clone_raw(_)), decrement_strong_count(_) = drop(from_raw(_)). However, providing the same functionality for weak count is hard to do in that scheme -- Weak::clone_raw suggests you need to have a raw weak to clone -- so that's a strong argument for the naming given here.)

One other note: the exposure of this functionality sort of confirms that for the purposes of converting between *const T and Arc<T>, all *const T (derived from an originating Arc::into_raw) are equal, and they don't maintain ownership of a specific strong reference they're associated to. (I.e. a = Arc::new(_); b = Arc::into_raw(a.clone()); c = Arc::into_raw(a); Arc::from_raw(b); Arc::from_raw(b); is fine.) I think this was intentionally implied by current documentation (and don't see any reasonable argument why it shouldn't be this way), but these functions require this behavior, and help to cement the model.


One doc clarification request: make it clearer that decrease_strong_count will drop the innards if it's the last strong count being freed, before digging into the Safety heading. (It could be intuitively assumed to just touch the strong count and not handle the final drop; another argument for spelling it drop(from_raw).) Also,

but should not be called after the final Arc has been released [emphasis original]

should probably be "must not" to avoid confusion.

Contributor

SimonSapin commented 4 days ago

Sorry I’m so late in the process but I feel this design is flawed.

Reasoning in terms of counter manipulation can lead to off-by-one errors, I think it’s better (especially with Rust’s move semantics) to consider a strong reference as a resource whose ownership can be transferred (or even borrowed, with &Arc<T>). Like Box::from_raw, Arc::from_raw takes ownership away from its input pointer such that it shouldn’t be used anymore.

The motivation discussed in #68700 (comment) is that unlike Box, with Arc we can and in some cases want to keep the pointer valid and make a new strong reference out of it instead of "consuming" it. Yes using Arc::clone + mem::forget is a clunky and confusing way to do this, but I feel that Arc::increment_strong_count is also unsafely patching things up at a lower level than necessary.

I don’t see a legitimate use of increment_strong_count that is not paired together with Arc::from_raw. Is there one? If not I feel that we’d be better off not exposing direct counter manipulation and instead add a variation of from_raw with the semantics desired here in terms of strong reference ownership.

impl<T: ?Sized> Arc<T> {
    pub unsafe fn clone_from_raw(ptr: *const T) -> Arc<T> {…}
}

(As to decrement_strong_count, it looks like it was mostly added for symmetry?)

This might not work, but:

@rfcbot concern clone_from_raw instead

The RawWaker API would need to forget the resulting Arc rather than making use of it in the clone "method". I think most use cases for this API aren't actually creating Arcs directly. The uses I've had historically have also done similar things to the RawWaker API, for example across the FFI boundary where either side wants to increment the counter on a shared resource, due to cloning, but isn't actually going to pass back the Arc (because nothing is going to interact with the Arc directly). That said I don't disagree that the clone_from_raw API is in some cases a bit cleaner; I think I would rather add both though. These methods are largely aliases for 1-3 lines of code, we can expose more than one incrementing method if we want. But I think there are valid uses for both just incrementing and for incrementing and getting a reference.

Member

m-ou-se commented yesterday

@SimonSapin I think you have to be on rfcbot's list to add concerns.

@rfcbot concern clone_from_raw instead

#79285 (comment)


About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK