Add a comment to Arc::MAX_REFCOUNT

[dsprenkels]

Before, the strong count of an Arc could be set to
MAX_REFCOUNT + 1, because when this happened, old_size would
be exactly MAX_REFCOUNT. Arc::clone() would not abort.

This commit changes the check in Arc::clone() to also abort if
the old value is equal to MAX_REFCOUNT, because then the new
value will be equal to MAX_REFCOUNT + 1.

A test would require allocating memory for isize::MAX pointers.
This would probably crash any machine, so no test is submitted
with this commit.

[rust-highfive]

r? @brson

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

[hanna-kruppe]

A test would require allocating memory for isize::MAX pointers.
This would probably crash any machine, so no test is submitted
with this commit.

You don't need to keep them all in memory, they can be fed into std::mem::forget. But this would still take a lot of time (astronomically so on 64 bit platforms) unless the compiler happens to optimize the loop out entirely.

[dsprenkels]

I see you're right.

The following code runs in 10 seconds on my laptop:

use std::i32;
use std::mem::forget;
use std::sync::Arc;

fn main() {
    let three = Arc::new(3);
    for _ in 0..i32::MAX {
        let x = three.clone();
        forget(x);
    }
    println!("done");
}

So for 32 bit systems, testing this would be feasible. Should I add a test which only runs on 32 bit systems?

[alexcrichton]

Thanks for the PR! I'm not sure I quite understand the change, though, or why it's necessary. The reference count can get much higher than isize::MAX because additions happen before we check what it was.

The "problem" comes about when the refcount hits usize::MAX, and we're postulating that because isize::MAX is about halfway there it'll be next to impossible for the refcount to jump that high before the program aborts. Does that make sense?

[dsprenkels]

@alexcrichton The reference count can't indeed get much higher thatn isize::MAX. I saw the constant MAX_REFCOUNT. I assume MAX_REFCOUNT is the maximum reference count, hence the name. Then the reference count should never be able to exceed this value.

Currently it is possible to exceed the maximum refcount (as defined by the constant). This should not be possible (?).

If this was meant to be just a soft limit, this PR is probably meaningless.

[alexcrichton]

Yeah this may just be a badly named constant. There's no actual need to prevent the refcount from going above this constant, it's just an indicator that calls to clone will start to try to abort the process as soon as they cross this threshold.

[dsprenkels]

I think it's in order to do some kind of annotation in the code to inform the reader about this. Right now it kinda looks like the programmer has made an off-by-one typo in line 463.

I could (instead of the proposed change) add a comment at the declaration of MAX_REFCOUNT.

[alexcrichton]

Yeah so to be clear the code as written I believe is working as intended, so there's no burning need to change it. Both before and after this patch are functionally ok, but the patch itself isn't required.

I'd be fine with renaming the variable perhaps though or adding a comment.

[dsprenkels]

I undid the original changes and added a comment to MAX_REFCOUNT.

[alexcrichton]

@bors: r+ rollup

[bors]

📌 Commit 99aaccd has been approved by alexcrichton

[Mark-Simulacrum]

Would it be possible to update the PR title and description to match what the PR now does? Since both are copied into commit history by bors/homu, it would be nice to have them up-to-date.

Not critical though, so just a suggestion.

[bors]

⌛ Testing commit 99aaccd with merge 045a727...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: alexcrichton
Pushing 045a727 to master...