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

Instant::checked_duration_since #58395

Merged
merged 3 commits into from
Feb 17, 2019
Merged

Conversation

vi
Copy link
Contributor

@vi vi commented Feb 12, 2019

No description provided.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2019
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@Centril
Copy link
Contributor

Centril commented Feb 12, 2019

cc #58402

@vi vi changed the title Augument #55527 with checked_duration_since. Instant::checked_duration_since Feb 12, 2019
@rust-highfive

This comment has been minimized.

@vi
Copy link
Contributor Author

vi commented Feb 12, 2019

How tests are supposed to be added for fresh #[unstable] items?

@Centril
Copy link
Contributor

Centril commented Feb 12, 2019

@vi You add #![feature(checked_duration_since)] to the crate root of libstd.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:06ae2b3e:start=1550008356428108442,finish=1550009002085614826,duration=645657506384
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:10:41] 
[01:10:41] running 119 tests
[01:11:06] .iiiii...i.....i..i...i..i.i..i.ii...i.....i..i....i..........iiii..........i...ii...i.......ii.i.i. 100/119
[01:11:11] i......iii.i.....ii
[01:11:11] 
[01:11:11]  finished in 29.961
[01:11:11] travis_fold:end:test_debuginfo

---
[01:30:17] .................................................................................................... 600/988
[01:30:25] .................................................................................................... 700/988
[01:30:34] .........iiii....................................................................................... 800/988
[01:30:46] .................................................................................................... 900/988
[01:30:53] ...................................iiii...................................F.............
[01:30:53] 
[01:30:53] ---- time.rs - time::Instant::checked_duration_since (line 226) stdout ----
[01:30:53] ---- time.rs - time::Instant::checked_duration_since (line 226) stdout ----
[01:30:53] error[E0658]: use of unstable library feature 'checked_duration_since' (see issue #58402)
[01:30:53]   --> time.rs:233:26
[01:30:53]    |
[01:30:53] 10 | println!("{:?}", new_now.checked_duration_since(now));
[01:30:53]    |
[01:30:53]    |
[01:30:53]    = help: add #![feature(checked_duration_since)] to the crate attributes to enable
[01:30:53] 
[01:30:53] error[E0658]: use of unstable library feature 'checked_duration_since' (see issue #58402)
[01:30:53]   --> time.rs:234:22
[01:30:53]    |
[01:30:53] 11 | println!("{:?}", now.checked_duration_since(new_now)); // None
[01:30:53]    |
[01:30:53]    |
[01:30:53]    = help: add #![feature(checked_duration_since)] to the crate attributes to enable
[01:30:53] thread 'time.rs - time::Instant::checked_duration_since (line 226)' panicked at 'couldn't compile the test', src/librustdoc/test.rs:354:13
[01:30:53] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:30:53] 
[01:30:53] 
---
[01:30:53] 
[01:30:53] error: test failed, to rerun pass '--doc'
[01:30:53] 
[01:30:53] 
[01:30:53] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std" "--" "--quiet"
[01:30:53] 
[01:30:53] 
[01:30:53] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:30:53] Build completed unsuccessfully in 0:31:55
[01:30:53] Build completed unsuccessfully in 0:31:55
[01:30:53] make: *** [check] Error 1
[01:30:53] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1e3222fc
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue Feb 12 23:34:24 UTC 2019
---
travis_time:end:0df0d5e7:start=1550014466395662680,finish=1550014466401999081,duration=6336401
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:126c2048
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:132c4a9a
travis_time:start:132c4a9a
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:083d6ad7
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

/// # Examples
///
/// ```no_run
/// use std::time::{Duration, Instant};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// use std::time::{Duration, Instant};
/// #![feature(checked_duration_since)]
/// use std::time::{Duration, Instant};

#[test]
fn checked_instant_duration_nopanic() {
let a = Instant::now();
(a - Duration::new(1, 0)).checked_duration_since(a);
Copy link
Member

Choose a reason for hiding this comment

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

nit: probably worth checking that it's None, too.

@scottmcm
Copy link
Member

Random thought: would saturating_duration_since make sense too?

@vi
Copy link
Contributor Author

vi commented Feb 13, 2019

@scottmcm I was also thinking about returning Result<Duration,Duration>, but decided to keep it the same as with other checked_*.

Another idea, for laziers who don't want to remember which one is earlier and which one is later:

    /// Returns the amount of time elapsed from another instant to this one,
    /// or from this one to another one, whichever is non-negative.
    pub fn distance(&self, another: Instant) -> Duration;

I'll probably add saturating_duration_since, returning zero duration instead of None.

Is wrong order of Instants the only overflow situation possible in duration_since or it can also happen if two Instants are too far apart?

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Could you explain more about the concrete use case for this? The usual use of Instant is for measuring duration between a "start" and "end" instant, in which you always know which one is first.

I think #55527 was well justified:

Since SystemTime is opaque there is no way to check if the result of an addition will be in bounds. That makes the Add<Duration> trait completely unusable with untrusted data. This is a big problem because adding a Duration to UNIX_EPOCH is the standard way of constructing a SystemTime from a unix timestamp.

but there doesn't seem to be a similar compelling justification in #58402.

@dtolnay
Copy link
Member

dtolnay commented Feb 14, 2019

Another idea, for laziers who don't want to remember which one is earlier and which one is later

I agree that these are things we could add, but I would prefer not to pack the standard library with public signatures that will only ever get called by the test suite of the standard library. I would like for anything we add to be justified more strongly than this.

@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 14, 2019
@vi
Copy link
Contributor Author

vi commented Feb 15, 2019

@dtolnay, The reason is similar to checked_add case: untrusted input.

Plain Instant::duration_since is OK when both Instants come from Instant::now, but when one or both instants are actually some baseline Instant offset by a constructed Duration based on untrusted input, order becomes less reliable, hence the need for checked and saturating version.

For example, in some experiment spanning both local and remote host, there may be multiple Instants, signifying moments of time something happened locally or remotely. For calculation of duration between a local moment and some remote moment it is a good reason to use checked arithmetic.

@dtolnay
Copy link
Member

dtolnay commented Feb 15, 2019

Hmm, I am still not seeing it. What I question is not whether it's possible to provoke a panic from duration_since, but whether there would ever be code that is expressed most clearly using checked_duration_since / saturating_duration_since rather than some other way. Would you personally want to use either of these methods today in existing code if they were available and stable? Separately, would you be able to point out some existing real-world code where these methods are the clearest solution?

@vi
Copy link
Contributor Author

vi commented Feb 15, 2019

Most recent example from my code:

let rtt4 = if now >= send_time { now - send_time } else { Duration::from_secs(999)};

now.saturating_duration_since(send_time) would probably be OK for this, or a checked_duration_since with more explicit fallback.

The comparison was added when I suddenly encountered a panic.

A close call nearby:

if now < deadline {
    sleeper.sleep(deadline - now);
}

Although readable as is, using saturating_duration_since would be almost equally good.

That comparison is easy to miss, leading to panic when under load. Seeing checked/saturating analogues nearby in documentation puts one in "here be dragonsoverflows" mode and suggests care.


Searching Github for duration_since in Rust code leads mostly to SystemTime::duration_since which returns Result, so is more like a checked_duration_since.

Other code I found uses Instant just as a stopwatch to measure speed of some code span, not as more involved abstraction of a moment of time.

@dtolnay
Copy link
Member

dtolnay commented Feb 15, 2019

From your code:

let now = Instant::now();
if now > deadline {
    break;
}

let s: u64 = /* untrusted */;
let ns: u32 = /* untrusted */;

let since_base = Duration::new(s, ns);
let send_time = time_base + since_base;
let rtt4 = if now >= send_time { now - send_time } else { Duration::from_secs(999) };

This wouldn't be entirely fixed by checked_duration_since because the Duration::new and the time_base + since_base can both overflow and panic. I might write this securely as:

let s = Duration::from_secs(/* untrusted */);
let ns = Duration::from_nanos(/* untrusted */);

let to_send = s.checked_add(ns).unwrap_or(...);
let to_now = now - time_base;
let rtt4 = to_now.checked_sub(to_send).unwrap_or(...);

Maybe this suggests a best practice of sticking as much as possible to Instant::elapsed + subtraction of a trusted instant, and doing untrusted work only in terms of Duration which has a less dangerous API. What do you think?

Thanks for the example use case -- I can see from this code that Instant could benefit from a carefully designed checked API.

I think there is also room for a crate called easytime or something like that which provides Instant and Duration wrapper types with all the Add/Sub impls but that never panic, instead keeping track of whether overflow occurred. This would make the obvious code also correct without having to be careful about where all the checked_subs go:

use easytime::{Duration, Instant};


let s: u64 = /* untrusted */;
let ns: u32 = /* untrusted */;

let since_base = Duration::new(s, ns);
let rtt4 = now - (time_base + since_base);

// some method to go from easytime::Duration to std::time::Duration:
let rtt4 = rtt4.unwrap_or(...);

@dtolnay dtolnay added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 15, 2019
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I think these methods are good. Sorry for giving you grief! I copied your rationale over to the tracking issue.

I would still be interested in seeing an easytime crate to make this stuff easier to write correctly.

@dtolnay
Copy link
Member

dtolnay commented Feb 15, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Feb 15, 2019

📌 Commit 91f67fd has been approved by dtolnay

@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 Feb 15, 2019
@vi
Copy link
Contributor Author

vi commented Feb 15, 2019

Maybe this suggests a best practice of sticking as much as possible to Instant::elapsed + subtraction of a trusted instant, and doing untrusted work only in terms of Duration which has a less dangerous API. What do you think?

I want to reason in terms of moments of time and timespans. Although it is obviously possible to use Durations as moments of time (by always keeping in mind the base), it loses typestate protection against adding two Instants together and also keeps code less self-documenting (i.e. /// This Duration is actually a moment of time, not a duration of something).

@scottmcm
Copy link
Member

I would still be interested in seeing an easytime crate to make this stuff easier to write correctly.

Hmm, we have num::Wrapping to call wrapping_* methods. What's your first instinct about a num::Checked to call all the checked_* methods? (I was thinking it'd be a newtype over an option.)

kennytm added a commit to kennytm/rust that referenced this pull request Feb 16, 2019
bors added a commit that referenced this pull request Feb 16, 2019
Rollup of 19 pull requests

Successful merges:

 - #57929 (Rustdoc remove old style files)
 - #57981 (Fix #57730)
 - #58074 (Stabilize slice_sort_by_cached_key)
 - #58196 (Add specific feature gate error for const-unstable features)
 - #58293 (Remove code for updating copyright years in generate-deriving-span-tests)
 - #58306 (Don't default on std crate when manipulating browser history)
 - #58359 (librustc_mir: use ? in impl_snapshot_for! macro)
 - #58395 (Instant::checked_duration_since)
 - #58429 (fix Box::into_unique effecitvely transmuting to a raw ptr)
 - #58433 (Update which libcore/liballoc tests Miri ignores, and document why)
 - #58438 (Use posix_spawn_file_actions_addchdir_np when possible)
 - #58440 (Whitelist the ARM v6 target-feature)
 - #58448 (rustdoc: mask `compiler_builtins` docs)
 - #58468 (split MaybeUninit into several features, expand docs a bit)
 - #58477 (Fix the syntax error in publish_toolstate.py)
 - #58479 (compile-pass test for #53606)
 - #58489 (Fix runtime error in generate-keyword-tests)
 - #58496 (Fix documentation for std::path::PathBuf::pop)
 - #58509 (Notify myself when Clippy toolstate changes)
kennytm added a commit to kennytm/rust that referenced this pull request Feb 17, 2019
bors added a commit that referenced this pull request Feb 17, 2019
Rollup of 19 pull requests

Successful merges:

 - #57929 (Rustdoc remove old style files)
 - #57981 (Fix #57730)
 - #58074 (Stabilize slice_sort_by_cached_key)
 - #58196 (Add specific feature gate error for const-unstable features)
 - #58293 (Remove code for updating copyright years in generate-deriving-span-tests)
 - #58306 (Don't default on std crate when manipulating browser history)
 - #58359 (librustc_mir: use ? in impl_snapshot_for! macro)
 - #58395 (Instant::checked_duration_since)
 - #58429 (fix Box::into_unique effecitvely transmuting to a raw ptr)
 - #58433 (Update which libcore/liballoc tests Miri ignores, and document why)
 - #58438 (Use posix_spawn_file_actions_addchdir_np when possible)
 - #58440 (Whitelist the ARM v6 target-feature)
 - #58448 (rustdoc: mask `compiler_builtins` docs)
 - #58468 (split MaybeUninit into several features, expand docs a bit)
 - #58479 (compile-pass test for #53606)
 - #58489 (Fix runtime error in generate-keyword-tests)
 - #58496 (Fix documentation for std::path::PathBuf::pop)
 - #58509 (Notify myself when Clippy toolstate changes)
 - #58521 (Fix tracking issue for error iterators)
@bors bors merged commit 91f67fd into rust-lang:master Feb 17, 2019
@dtolnay
Copy link
Member

dtolnay commented Mar 1, 2019

@scottmcm:

What's your first instinct about a num::Checked to call all the checked_* methods?

I think it would be less good than https://github.com/taiki-e/easytime.

I think construction of Checked would be confusing or complicated:

// not panic-free
let _ = Checked::new(Duration::new(s, ns));

// weird and not panic-free
let _ = Checked(Some(Duration::new(s, ns)));

// :( less important words up front
let _ = Checked::new_duration(s, ns);

// ???
let _ = Checked::now();
let _ = Checked::<Instant>::now();
let _ = Checked::now_instant();
let _ = Checked::instant_now();

// fine...
// except people would write Checked::new(s, ns)
// which is confusing and breaks when we later add `new` method for another type
let _ = Checked::<Duration>::new(s, ns);

// checked_ prefix implies it returns Option, not Checked<Duration>
let _ = Duration::checked_new(s, ns);

// not bad...
let _ = Duration::new_checked(s, ns);

// ... but for Instant?
let _ = Instant::now_checked(); // ???

// easytime
let _ = Duration::new(s, ns);
let _ = Instant::now();

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.

6 participants