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

SystemTime does not discuss leap seconds in docs #77994

Closed
ijackson opened this issue Oct 15, 2020 · 8 comments · Fixed by #109660
Closed

SystemTime does not discuss leap seconds in docs #77994

ijackson opened this issue Oct 15, 2020 · 8 comments · Fixed by #109660
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ijackson
Copy link
Contributor

SystemTime ought to explain what happens with leap seconds.

In particular, the current text under SystemTime::UNIX_EPOCH suggests using .duration_since() to get "Using duration_since on an existing SystemTime instance can tell how far away from this point in time a measurement lies".

A programmer who is reading this and does not know of the existence of leap seconds will assume that the result is a Unix time_t (with additional precision). I think this is in fact the case, but this is not stated and it should be.

I am filing this as a bug report because the lack of an explicit statement in the documentation makes it impossible to confidently write code which receives or emits unix times, even though that does seem to be the point of the UNIX_EPOCH constant.

It would probably be useful to mention leap seconds as a reason why the clock might go backwards or might not run at a constant rate, along with clock adjustments (the implications of which are discussed without naming them!)

@ijackson ijackson added the C-bug Category: This is a bug. label Oct 15, 2020
@camelid camelid added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 15, 2020
@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed C-bug Category: This is a bug. labels Oct 15, 2020
@frewsxcv
Copy link
Member

To document, we need to know what the actual behavior is. Does anyone know what happens if the clock goes back in time and we try to return the number of milliseconds?

@Ekleog
Copy link

Ekleog commented Jan 30, 2023

Coming here after looking for this from chronotope/chrono#21 ; I just want to say that the question, rather than "what is the current behavior", should probably be "what should the behavior be" IMO. The current docs explicitly state that SystemTime::now().duration_since(UNIX_EPOCH) should return the time elapsed since the unix epoch according to the current system clock. If the system clock is using a timezone that has leap seconds, then I don't see why libstd should be allowed to ignore them when computing the duration.

Does what I'm saying make sense? If so I may be willing to try to check it out and work on fixes for platforms that (I can access and) can be fixed properly, and workarounds for platforms that cannot; though I can't promise a timeline.

BTW, I'm particularly interested in SystemTime::now().duration_since(UNIX_EPOCH), as ensuring it returns a well-defined duration is needed for chrono to be correct (both now and with upcoming changes to make it handle leap seconds better), though whether it is always off-by-the-number-of-leap-seconds does not matter much in fixing chrono as chrono could offset the other way around.

(Also, leap seconds don't go in the past, though NTP adjustment can definitely make the clock go in the past)

@ijackson
Copy link
Contributor Author

The behaviour should be that SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs() returns the same numerical value as libc::time on Unix platforms. This is the time interface offered by other programming languages, and it is the current behaviour of the Rust stdlib. If you want to change this, I think it would warrant an RFC. I think it would be a very bad idea.

Leap seconds are extremely inconvenient to work with. They are inherently unpredictable, difficult to get reliable information about (existence of some tables and crates notwithstanding). They are also in the process of being abolished, perhaps even before the next one occurs.

OTOH the "system time with smeared leap seconds" approach nowadays taken by most common operating systems when synced to network time, works really well. For applications where precise real-time timing is required, Instant is more suitable since it actually works correctly.

@Ekleog
Copy link

Ekleog commented Jan 30, 2023

The behaviour should be that SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs() returns the same numerical value as libc::time on Unix platforms

It is not what is currently documented as the behavior of SystemTime, as SystemTime::now().duration_since(UNIX_EPOCH) is currently documented to be the duration since the unix epoch, which is definitely not libc::time due to leap seconds.

While I see your point (and don't really care what the end-choice is, as converting between the two is not too hard), I do think changing the documentation this significantly would also require an RFC, because the current documentation of duration_since would become just wrong otherwise. (And I would personally find it hard to actually document what duration_since is supposed to do if it's supposed to be the same as libc::time)

Basically there are two things that make sense to have. The current documentation claims one, the current implementation… probably is the other? And we need to reconcile the two.

@Ekleog
Copy link

Ekleog commented Jan 30, 2023

They are also in the process of being abolished, perhaps even before the next one occurs.

They are not being abolished, the occurrence of new leap seconds is going to be put off. There will still be a ~30-45s offset between UTC and TAI between 2035 and 2135; and duration_since will still have to be aware of it in order to choose whether it wants to return the duration with or without this offset.

But yes, I'm totally with you that even if the application crashes with leap seconds every time it happens, it's probably not too big a deal anyway, except at the google scale where they had to introduce UTC-SLS because it was too much for them; and I personally don't care that much about google scale. (Edit: Re-reading this, it sounds bad. I'm entirely first-degree here, I really don't care about whether there are leap seconds or not in SystemTime::now, and an offset of 30 seconds won't change my life. I really just care that the documentation and behavior match properly, because otherwise google-scale would be unable to work around whatever issues are raised by the behavior because they wouldn't know about it. They did UTC-SLS once, they can handle it again.)

The only thing I really care about is that the documentation and behavior of duration_since correctly match, then the people who write code using it can decide whether they want to adjust to leap second or not :) My message was just written to say "we should decide what we want the behavior to be, not document what the current behavior is, because documentation has been around for long enough for it to be a non-explicit spec, and because the documentation is a behavior that actually makes sense too"

@fanf2
Copy link

fanf2 commented Jan 30, 2023

There are a number of crates that assume SystemTime::now().duration_since(UNIX_EPOCH) is POSIX seconds since the epoch, e.g. humantime and my leapseconds table crate.

Outside POSIX APIs, there are protocols that use POSIX seconds since the epoch, e.g. JWT, CBOR, TOTP, certificate transparency, TSIG, DNSSEC, etc usw, and NTP timestamps are the same as POSIX time with a 70 year (2,208,988,800 second) offset. So POSIX-style seconds counts (ignoring leap seconds) are the norm, much more common than seconds counts that include leap seconds. Most unix-like operating systems do not even have a clock that counts time including leap seconds. (You can't use CLOCK_MONOTONIC because sometimes it can stop or even go backwards.)

And POSIX time is much more robust: it's just a formula from a broken-down time (in some flavour of UT, officially UTC) to an integer. This means it can trivially represent times in UT before the start of UTC, or in the 1960s rubber seconds period of UTC, or more than 11 months in the future, all of which are outside the range of a leap second table. Even within the range of a leap second table, POSIX time does not depend on dynamic data that is hard to obtain and keep up-to-date in a reliable and portable manner.

@Ekleog
Copy link

Ekleog commented Jan 30, 2023

Disclaimer before my message: Again, I agree that posix seconds since epoch is definitely the current de-facto standard in computers, though I disagree that posix is more robust in any way as literally any computer clock is counting TAI and not UTC unless there is software actively messing with leap seconds. Portability that said is definitely a matter, that makes TAI-based timekeeping hard, especially at the standard library level.

However, "POSIX seconds since the epoch" is not actually something that has a meaning. POSIX time has a meaning, and matches one point in real-life time with one integer that is not a number of seconds since the epoch. (Nor is it actually UTC, if I can trust djb's claims)

If the thing were returning a PosixDuration where the "duration" is defined by subtracting posix times, then that would definitely make sense to me, because PosixDuration is not a type that people would expect holds an actual number of second, but rather some imaginary number that happens to mostly match seconds and subseconds. I would even argue against having conversion functions between Duration and PosixDuration so people don't risk mistaking the two, and having the documentation of PosixDuration be very explicit.

Unfortunately, this is not something that can happen today. Honestly, the thing I would personally be most happy with would be deprecating duration_since with its current behavior, and adding a posix_duration_since that counts pseudo-durations in UTC time but returning a PosixDuration, that's basically the same but explicitly not a number of seconds, thus leaving Duration for Instant. I wonder what people would think about that third option (deprecation), that I hadn't thought of when writing we need to choose between fixing the code and fixing the documentation?

@ramon-garcia
Copy link

The current behavior is that SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_secs() matches libc::time() . I think that it should continue being so.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 28, 2023
Document that SystemTime does not count leap seconds

Fixes rust-lang#77994

This may not be entirely uncontroversial.  I know that `@Ekleog` is going to disagree.  However, in support of this docs change:

 This documents the current behaviour.  The alternative would be to plan to *change* the behaviour.

There are many programs which need to get a POSIX time (a `time_t`).  Right now, `duration_since(UNIX_EPOCH)` is the only facility in std that does that.  So, that is what programs use.  Changing the behaviour would  break[1] all of those programs.  We would need to define a new API that can be used to get a POSIX time, and get everyone to use it.  This seems highly unpalatable.

And, even if we wanted to do that, time with leap seconds is a lot less easy to work with.  We would need to arrange to have a leap seconds table available to `std` somehow, and make sure that it was kept up to date.  Currently we don't offer to do that for timezone data, which has similar needs.  There are other complications.  So it seems it would be awkwarrd to *implement* a facility that provides time including leap seconds, and the resulting value would be hard for applications to work with.

Therefore, I think it's clear that we don't want to plan to ever change `SystemTime`.  We should plan to keep it the way it is.  Providing TAI (for example) should be left to external crates, or additional APIs we may add in the future.

For more discussion see rust-lang#77994 and in particular `@fanf2's` rust-lang#77994 (comment)

[1]  Of course, by "break" we really only mean *future* breakage in the case where there is, in fact, ever another leap second.  There may well not be: they are in the process of being abolished (although this is of course being contested).  But if we decide that `SystemTime::now().duraton_since(UNIX_EPOCH)` counts leap seconds, it would start to return `Durations`s that are 27s different to the current answers.   That's clearly unacceptable.  And we can hardly change `UNIX_EPOCH` by 27s.
@bors bors closed this as completed in e329b23 Aug 28, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Aug 29, 2023
Document that SystemTime does not count leap seconds

Fixes #77994

This may not be entirely uncontroversial.  I know that `@Ekleog` is going to disagree.  However, in support of this docs change:

 This documents the current behaviour.  The alternative would be to plan to *change* the behaviour.

There are many programs which need to get a POSIX time (a `time_t`).  Right now, `duration_since(UNIX_EPOCH)` is the only facility in std that does that.  So, that is what programs use.  Changing the behaviour would  break[1] all of those programs.  We would need to define a new API that can be used to get a POSIX time, and get everyone to use it.  This seems highly unpalatable.

And, even if we wanted to do that, time with leap seconds is a lot less easy to work with.  We would need to arrange to have a leap seconds table available to `std` somehow, and make sure that it was kept up to date.  Currently we don't offer to do that for timezone data, which has similar needs.  There are other complications.  So it seems it would be awkwarrd to *implement* a facility that provides time including leap seconds, and the resulting value would be hard for applications to work with.

Therefore, I think it's clear that we don't want to plan to ever change `SystemTime`.  We should plan to keep it the way it is.  Providing TAI (for example) should be left to external crates, or additional APIs we may add in the future.

For more discussion see #77994 and in particular `@fanf2's` rust-lang/rust#77994 (comment)

[1]  Of course, by "break" we really only mean *future* breakage in the case where there is, in fact, ever another leap second.  There may well not be: they are in the process of being abolished (although this is of course being contested).  But if we decide that `SystemTime::now().duraton_since(UNIX_EPOCH)` counts leap seconds, it would start to return `Durations`s that are 27s different to the current answers.   That's clearly unacceptable.  And we can hardly change `UNIX_EPOCH` by 27s.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants