Description
The following piece of documentation from std::time::Instant
, which comes from PR #72836 via issue #48980, does not make sufficient sense and is not sufficiently helpful:
/// # OS-specific behaviors
///
/// An `Instant` is a wrapper around system-specific types and it may behave
/// differently depending on the underlying operating system. For example,
/// the following snippet is fine on Linux but panics on macOS:
///
/// ```no_run
/// use std::time::{Instant, Duration};
///
/// let now = Instant::now();
/// let max_nanoseconds = u64::MAX / 1_000_000_000;
/// let duration = Duration::new(max_nanoseconds, 0);
/// println!("{:?}", now + duration);
/// ```
The parameters to Duration::new()
are the number of seconds and the number of nanoseconds. Here we seem to be giving a quantity in nanoseconds as the number of seconds, and with that quantity representing... well, from the name max_nanoseconds
and calculation, the maximum number of seconds that can fit into a u64
if it later it is to be converted to a u64
quantity of nanoseconds without overflowing... huh :S The reviewers of PR #72836 seem to have overlooked this problem.
This text and example are there due to the fact that the internal attributes of Instant
differ per different platforms, and so a large Duration
added to an Instant
on one platform might work, while on another it might overflow and thus panic.
Here it follows that u64::MAX / 1_000_000_000
as the number of seconds in a Duration
just happens to be too large for addition to an Instant
on some platforms - macOS. The problem with the example fundamentally is the misleading variable name and possibly poor choice of chosen value.
One further problem with the text is that although it is helpful in pointing out that there is a difference in behaviour, it leaves the reader to have to guess at what the cause of the different behaviour is, and it does not give any indication of what range of values can typically be added to it without encountering failure.
I don't know why Instant
has been designed to use different underlying attributes rather than something consistent which would avoid it suffering from this unfortunate inconsistency, but I'll leave that for separate discussion / resolution, which already seems to have gone no-where in #48980, though #44394 for SystemTime
is still open.
Tasks to address this issue:
- Replace the code example with code that makes sense.Improve the text to better explain the cause of the difference.Improve the text to give guidance on what approximate size of values will trigger failure.
Activity
jnqnfe commentedon Dec 10, 2020
To address the first issue, we could start with this:
The original
u64::MAX / 1_000_000_000
is approximately18_446_744_073
. We could thus just round to20_000_000_000
, which is well within theu64
range and being a fraction larger than that in the example will still trigger the issue. We ditch themax_nanoseconds
nonsense with this change.Alternatively, we could try to calculate something possibly closer to the point of overflow on macOS. If the internal
u64
value on macOS represented nanoseconds, this would put the max value before overflow around 584 years, and thus it might be nice to use a nice round 1000 years:Which actually turns out to be larger than the previous value, but
millenia_in_secs
is nicer than just a rather arbitrarybig_chunk_of_time
.However the value within
Instant
on macOS does not represent nanoseconds; as discussed here it's a "mach absolute time unit" value. A calculation needs to be performed upon it to convert to nanoseconds, as done if/when you try to add or subtract a duration or subtract anotherInstant
. (This issue of inconsistency would not be an issue if only that calculation was done up front upon creating theInstant
and converted nanoseconds to { secs, nanos }). This thus complicates figuring out a suitably closer value to use in the example, and complicates task item 3, along with myself not having macOS devices to test things on. Howevermillenia_in_secs
is quite nice and not hugely different to what was already determined to trigger things and the previous discussion in #48980 suggested that values up to hundreds of years should work, which seems correct, so that gives somewhat of a rough guide that could be given for task 3.All three task items could perhaps be resolved with something like this perhaps:
Perhaps a fourth task item could be to ensure that there's a test that enforces that durations up to 100 years can be added successfully on all platforms?
thomcc commentedon Dec 10, 2020
This feels like it would be somewhat out of our control if it were to change, and also doesn't seem like a very useful thing to require for
Instant
. offsets of a hundred of year feels pretty clearly outside the realm of usefulness[0] for Instant.At least, it's not something that it feels like we'd want to make tradeoffs in favor of, nor is it one we'd want to be in the business of guaranteeing going forwards, I think.
On macOS we're at the mercy of the mach_absolute_time timebase (which apple has made very clear that people should be handling correctly, as it's going to change), on Windows by the QPF result, etc.
It seems like it would be a pretty unfortunate situation if we had to use a less ideal measurement for Instant in the future because we promised a particular range of values will work, and the better timestamp can't represent that range.
[0]: The only time I can think of it coming up is if the code wants to express a notion like "infinitely far in the future", which perhaps is an argument that it should be expressible some other way.
jnqnfe commentedon Dec 10, 2020
@thomcc, I completely agree that adding such large time durations is probably not going to be done in practice. In terms of the test, I was not suggesting that we provide any guarantee as you describe, I was suggesting that we provide a suggestion of what might be possible as per the last block of text I wrote, and use the test as a canary to warn us if the situation ever changes, such that we can know to update the documentation (and test) accordingly.
Edit: I mean this was what I meant to suggest.
Rollup merge of rust-lang#136301 - hkBst:patch-33, r=thomcc
Unrolled build for rust-lang#136301
Rollup merge of rust-lang#136301 - hkBst:patch-33, r=thomcc
Rollup merge of rust-lang#136301 - hkBst:patch-33, r=thomcc