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

Implement Weak::{strong_count, weak_count} #56696

Merged
merged 4 commits into from
Jan 31, 2019
Merged

Implement Weak::{strong_count, weak_count} #56696

merged 4 commits into from
Jan 31, 2019

Conversation

jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented Dec 10, 2018

The counters are also useful on Weak, not just on strong references (Rc or Arc).

In situations where there are still strong references around, you can also get these counts by temporarily upgrading and adjusting the values accordingly. Using the methods introduced here is simpler to do, less error-prone (since you can't forget to adjust the counts), can also be used when no strong references are around anymore, and might be more efficient due to not having to temporarily create an Rc.

This is mainly useful in assertions or tests of complex data structures. Data structures might have internal invariants that make them the sole owner of a Weak pointer, and an assertion on the weak count could be used to ensure that this indeed happens as expected. Due to the presence of Weak::upgrade, the strong_count becomes less useful, but it still seems worthwhile to mirror the API of Rc.

TODO:

Closes #50158

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 10, 2018
// Due to the implicit weak pointer added when any strong pointers are
// around, we cannot implement `weak_count` correctly since it necessarily
// requires accessing the strong count and weak count in an unsynchronized
// fashion.
Copy link
Contributor Author

@jonas-schievink jonas-schievink Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a strong_count method to sync::Weak as well, but here the tradeoff is less clear since (I think) weak_count can not be implemented correctly, so the API must stay asymmetric like this (or not offer strong_count at all, since it isn't very useful)

@Dylan-DPC-zz
Copy link

ping from triage, can anyone from @rust-lang/libs review this?

@alexcrichton
Copy link
Member

Having a discrepancy in weak_count is sort of a bummer, would it be possible to implement, even a racy version, on Arc? Otherwise shouldn't weak_count on a Weak always return >= 1?

@Centril
Copy link
Contributor

Centril commented Jan 25, 2019

r? @alexcrichton

@alexcrichton
Copy link
Member

Oops sorry for the delay, @jonas-schievink feel free to ping the PR when it's updated as notifications aren't always sent out.

I wonder if weak_count here should return Option<usize> where None is returned if all the strong references have been dropped and as a result the weak count may not be known?

@jonas-schievink
Copy link
Contributor Author

Weak::weak_count only returns 0 if the Weak was created via Weak::new(), strong references don't matter. I do agree that returning None in that case might be better since having a method on Weak tell you "there are no Weaks" is a bit contradictory.

Should the same be done for strong_count? That one is less contradictory, but "There are no strong pointers pointing to the value" (0) is still more wrong than returning "There is no value any strong pointer could point to" (None). Having strong_count still return usize would also make for an odd difference between two otherwise very similar methods.

@alexcrichton
Copy link
Member

Yeah I agree that Weak::new is the only problematic case to handle here. I think for strong_count though it's fine to infer zero because Weak::new does indeed imply there are only ever zero strong pointers!

@alexcrichton
Copy link
Member

Er to add some more, I'm not too worried about the inconsistency of return values, these are pretty niche methods so having them be absolutely consistent isn't so important

@alexcrichton
Copy link
Member

@bors: r+

Ok looks great to me to add, thanks @jonas-schievink!

@bors
Copy link
Contributor

bors commented Jan 31, 2019

📌 Commit 0d314f0 has been approved by alexcrichton

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2019
@bors
Copy link
Contributor

bors commented Jan 31, 2019

⌛ Testing commit 0d314f0 with merge f29b4fb...

bors added a commit that referenced this pull request Jan 31, 2019
Implement Weak::{strong_count, weak_count}

The counters are also useful on `Weak`, not just on strong references (`Rc` or `Arc`).

In situations where there are still strong references around, you can also get these counts by temporarily upgrading and adjusting the values accordingly. Using the methods introduced here is simpler to do, less error-prone (since you can't forget to adjust the counts), can also be used when no strong references are around anymore, and might be more efficient due to not having to temporarily create an `Rc`.

This is mainly useful in assertions or tests of complex data structures. Data structures might have internal invariants that make them the sole owner of a `Weak` pointer, and an assertion on the weak count could be used to ensure that this indeed happens as expected. Due to the presence of `Weak::upgrade`, the `strong_count` becomes less useful, but it still seems worthwhile to mirror the API of `Rc`.

TODO:
* [X] Tracking issue - #57977

Closes #50158
@bors
Copy link
Contributor

bors commented Jan 31, 2019

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

@bors bors merged commit 0d314f0 into rust-lang:master Jan 31, 2019
Centril added a commit to Centril/rust that referenced this pull request Dec 15, 2019
Stabilize `std::{rc,sync}::Weak::{weak_count, strong_count}`

* Original PR: rust-lang#56696
* Tracking issue: rust-lang#57977

Closes: rust-lang#57977

Supporting comments:

> Although these were added for testing, it is occasionally useful to have a way to probe optimistically for whether a weak pointer has become dangling, without actually taking the overhead of manipulating atomics. Are there any plans to stabilize this?

_Originally posted by @bdonlan in rust-lang#57977 (comment)

> Having this stabilized would help. Currently, the only way to check if a weak pointer has become dangling is to call `upgrade`, which is by far expensive.

_Originally posted by @glebpom in rust-lang#57977 (comment)

Not sure if stabilizing these warrants a full RFC, so throwing this out here as a start for now.

Note: per CONTRIBUTING.md, I ran the tidy checks, but they seem to be failing on unchanged files (primarily in `src/stdsimd`).
Centril added a commit to Centril/rust that referenced this pull request Dec 15, 2019
Stabilize `std::{rc,sync}::Weak::{weak_count, strong_count}`

* Original PR: rust-lang#56696
* Tracking issue: rust-lang#57977

Closes: rust-lang#57977

Supporting comments:

> Although these were added for testing, it is occasionally useful to have a way to probe optimistically for whether a weak pointer has become dangling, without actually taking the overhead of manipulating atomics. Are there any plans to stabilize this?

_Originally posted by @bdonlan in rust-lang#57977 (comment)

> Having this stabilized would help. Currently, the only way to check if a weak pointer has become dangling is to call `upgrade`, which is by far expensive.

_Originally posted by @glebpom in rust-lang#57977 (comment)

Not sure if stabilizing these warrants a full RFC, so throwing this out here as a start for now.

Note: per CONTRIBUTING.md, I ran the tidy checks, but they seem to be failing on unchanged files (primarily in `src/stdsimd`).
Centril added a commit to Centril/rust that referenced this pull request Dec 16, 2019
Stabilize `std::{rc,sync}::Weak::{weak_count, strong_count}`

* Original PR: rust-lang#56696
* Tracking issue: rust-lang#57977

Closes: rust-lang#57977

Supporting comments:

> Although these were added for testing, it is occasionally useful to have a way to probe optimistically for whether a weak pointer has become dangling, without actually taking the overhead of manipulating atomics. Are there any plans to stabilize this?

_Originally posted by @bdonlan in rust-lang#57977 (comment)

> Having this stabilized would help. Currently, the only way to check if a weak pointer has become dangling is to call `upgrade`, which is by far expensive.

_Originally posted by @glebpom in rust-lang#57977 (comment)

Not sure if stabilizing these warrants a full RFC, so throwing this out here as a start for now.

Note: per CONTRIBUTING.md, I ran the tidy checks, but they seem to be failing on unchanged files (primarily in `src/stdsimd`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants