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

time: Round(0), Truncate(0) strip monotonic clock readings but documentation still says it returns t unchanged #21485

Closed
glycerine opened this issue Aug 16, 2017 · 20 comments
Labels
CherryPickApproved Used during the release process for point releases Documentation FrozenDueToAge release-blocker
Milestone

Comments

@glycerine
Copy link

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.9rc2 darwin/amd64

this does not happen, for contrast, with go1.8.3 darwin/amd64

This change is documented at the top of the new go1.9rc2 time/time.go file:

// For debugging, the result of t.String does include the monotonic                                             
// clock reading if present. If t != u because of different monotonic clock readings,                           
// that difference will be visible when printing t.String() and u.String().                                     

This is actually a rather drastic proposed change. I refer not the use of the monotone clock, but the display of it in timestamps formated via String(). Many of us use String() for many purposes other than debugging. For debugging, a separate new StringWithMonotone() should be provided.

What operating system and processor architecture are you using (go env)?

go env:
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jaten/go"
GORACE=""
GOROOT="/usr/local/go1.9rc2"
GOTOOLDIR="/usr/local/go1.9rc2/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/6s/zdc0hvvx7kqcglg5yqm3kl4r0000gn/T/go-build055854381=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Here code to reproduce the issue, serialize_time.go. It is extracted from the time.Time serialization in a popular msgpack serialization package. e.g. getunix() and putunix() are here https://github.com/tinylib/msgp/blob/ad0ff2e232ad2e37faf67087fb24bf8d04a8ce20/msgp/integers.go#L113

package main

import (
	"fmt"
	"time"
)

func main() {

	// time to bytes
	t := time.Now()
	fmt.Printf("original  t='%s'\n", t.String())
	t = t.UTC()
	fmt.Printf("original  t='%s' in UTC\n", t.String())
	var b [12]byte
	putUnix(b[:], t.Unix(), int32(t.Nanosecond()))

	// bytes to time:
	sec, nsec := getUnix(b[:])
	t2 := time.Unix(sec, int64(nsec)).Local()

	fmt.Printf("restored t2='%s'\n", t2.String())
}

/* output of main:

original  t='2017-08-16 19:35:41.958759249 -0400 EDT m=+0.000241395'
original  t='2017-08-16 23:35:41.958759249 +0000 UTC' in UTC
restored t2='2017-08-16 19:35:41.958759249 -0400 EDT'

*/

func getUnix(b []byte) (sec int64, nsec int32) {
	sec = (int64(b[0]) << 56) | (int64(b[1]) << 48) |
		(int64(b[2]) << 40) | (int64(b[3]) << 32) |
		(int64(b[4]) << 24) | (int64(b[5]) << 16) |
		(int64(b[6]) << 8) | (int64(b[7]))

	nsec = (int32(b[8]) << 24) | (int32(b[9]) << 16) | (int32(b[10]) << 8) | (int32(b[11]))
	return
}

func putUnix(b []byte, sec int64, nsec int32) {
	b[0] = byte(sec >> 56)
	b[1] = byte(sec >> 48)
	b[2] = byte(sec >> 40)
	b[3] = byte(sec >> 32)
	b[4] = byte(sec >> 24)
	b[5] = byte(sec >> 16)
	b[6] = byte(sec >> 8)
	b[7] = byte(sec)
	b[8] = byte(nsec >> 24)
	b[9] = byte(nsec >> 16)
	b[10] = byte(nsec >> 8)
	b[11] = byte(nsec)
}

In summary, in go1.9rc2, time.Time.String() may produce:

2017-08-16 18:41:39.184829495 -0400 EDT m=+0.057013769

whereas in go1.83, the String() applied to the same value produces always:

2017-08-16 18:41:39.184829495 -0400 EDT

While not a language consistency issue, this is a backwards-compatibility issue for programs that expected one thing from their time.Time values. For example, my tests in a fork of the tinylib/msgp lib cited above are suddenly broken under go1.9rc2. So these changes may break many programs unexpectedly when moving from go1.8.3 to go1.9.

I'm glad for the new monotonic time feature under the covers, as it solves bugs with walltime going through leap seconds, but does it have to surface itself into the string representation of time in a non-backcompatible way? For the new info, how about adding a separate stringification method if one needs both timestamps? Perhaps: StringWithMonotone() alongside the old fashioned String().

In summary, while useful, features meant for debugging the new time.Time values should not break backwards compatibility, when they can trivially be provided alongside in a separate new method.

@glycerine glycerine changed the title go1.9rc2 stringifies time.Time in a backwards incompatible way, adding m=+0.000241395 go1.9rc2 stringifies time.Time in a backwards incompatible way, adding m=+0.000241395 Aug 17, 2017
@ianlancetaylor ianlancetaylor changed the title go1.9rc2 stringifies time.Time in a backwards incompatible way, adding m=+0.000241395 time: go1.9rc2 stringifies time.Time in a backwards incompatible way, adding m=+0.000241395 Aug 17, 2017
@ianlancetaylor
Copy link
Contributor

As the comment says, this is an intentional change, because without it you can have two time.Time values that are not == but print the same.

It would be hard to change it at this point in the release process. At the least we would want to hear whether anybody else is reporting a breakage. This is the first problem report I've seen on this.

CC @rsc

@glycerine
Copy link
Author

glycerine commented Aug 17, 2017

without it you can have two time.Time values that are not == but print the same.

Ah.

I think I should probably start serializing the monotone part as well....since the typically check on serialization correctness is to compare for equality with the original value after serialization and deserializing... hmm. Or start inserting StripMono() calls.

Since it looks like its inevitable, this would be helpful for patching code that wants to use 1.9: Perhaps stripMono() could be exported? I've developed the following workaround, but its a rather elaborate and an extra function call when stripMono() could simply be StripMono().

func StripMono(tm time.Time) time.Time {

  // If we're going to print t, but
  // we don't want to show the
  // m+0.057013769 monotone timestamp at the end...

  // Since t.stripMono() is private at the moment,
  // we have to	do the mystical incantation:

  return tm.In(tm.Location())

  // While it looks like the above should be a no-op, it has
  // the effect of calling t.stripMono() and so getting
  // rid of the monotone part of the String() representation.
  // Update: ugly and fragile b/c it depends on unspecified
  // behavior in the time API.
}

Actually it looks like Truncate(0) might be a single function call to
accomplish the same thing. But the docs (below) should probably be updated
to note that t is not returned unchanged, but rather has had the
monotonic timestamp portion stripped, and so will no longer be
equal to the input if the input had a non-zero monotone part.

// Truncate returns the result of rounding t down to a multiple of d (since the zero time).                     
// If d <= 0, Truncate returns t unchanged.                                                                     
//                                                                                                              
// Truncate operates on the time as an absolute duration since the                                              
// zero time; it does not operate on the presentation form of the                                               
// time. Thus, Truncate(Hour) may return a time with a non-zero                                                 
// minute, depending on the time's Location.                                                                    
func (t Time) Truncate(d Duration) Time {
    t.stripMono()
    if d <= 0 {
        return t
    }
    _, r := div(t, d)
    return t.Add(-r)
}

Or not. Actually, I'm reconsidering. It would make more sense to keep Truncate(0) API backwards compatible (move the stripMono after the test rather than before). After all, this is otherwise a change in the API. I'm sure there is code that depends upon its claim of If d <= 0, Truncate returns t unchanged that will break.

And just simply export StripMono.

@dmitshur
Copy link
Contributor

dmitshur commented Aug 17, 2017

Time.String documentation mentions:

for a stable serialized representation, use t.MarshalText, t.MarshalBinary, or t.Format with an explicit format string.

fmt.Stringer documentation says:

Stringer is implemented by any value that has a String method, which defines the “native” format for that value. The String method is used to print values passed as an operand to any format that accepts a string or to an unformatted printer such as Print.

Importantly, time package documentation says in multiple places that the Time.Equal method should be used to test whether two time.Time values represent the same time instant. Using == operator or String method has documented pitfalls.

I don't be think you can rely on Stringer output not changing between any Go versions. That would be akin to using reflect.DeepEqual and expecting tests not to break. It's not viable. Instead, alternatives that are guaranteed to produce stable marshaling or equality comparisons should be used.

@dsnet gave a very informative talk on this subject at this year's GopherCon, see https://www.youtube.com/watch?v=OuT8YYAOOVI.

@glycerine
Copy link
Author

I see that the go1.9rc2 time API documentation does recommend using Round(0) to strip the mono part; at https://github.com/golang/go/blob/master/src/time/time.go#L46

// The canonical way to strip a monotonic clock reading is to use t = t.Round(0).

However the documentation for Round itself belies this, also claiming at https://github.com/golang/go/blob/master/src/time/time.go#L1403 that

// If d <= 0, Round returns t unchanged.

@glycerine
Copy link
Author

Meta: The Round(0) choice just seems like poor API design; to overload the meaning of Round(0), when StripMono() would be clear and does exactly and only what it says. The former requires a trip to the documentation to read up what on earth Round(0) means by every new user. The later does not. Also then the meaning of Round(0) becomes constrained and cannot be changed to mean something more logical/relevant in the future.

@dmitshur
Copy link
Contributor

On the topic of Round(0) choice, see relevant issue #18991 for prior discussion.

@dsnet
Copy link
Member

dsnet commented Aug 17, 2017

This issue of String's output changing was brought up in #20876, and the decision there is to be more firm that String is intended for human consumption only, in which case there is no guarantees about stability.

As @shurcooL mentioned, MarshalText, MarshalBinary, or Format should be used for stable representations of time.

@dsnet dsnet closed this as completed Aug 17, 2017
@dsnet dsnet added this to the Go1.9 milestone Aug 17, 2017
@rsc
Copy link
Contributor

rsc commented Aug 17, 2017

@glycerine

Meta: The Round(0) choice just seems like poor API design; to overload the meaning of Round(0), when StripMono() would be clear and does exactly and only what it says. The former requires a trip to the documentation to read up what on earth Round(0) means by every new user. The later does not. Also then the meaning of Round(0) becomes constrained and cannot be changed to mean something more logical/relevant in the future.

All this is true, but if you write Round(0) then it will work in Go 1.8, so we avoid bifurcating the world of compiling Go programs.

@glycerine
Copy link
Author

we avoid bifurcating the world of compiling Go programs.

@rsc Very good rationale. Thanks for pointing that out.

@glycerine
Copy link
Author

glycerine commented Aug 17, 2017

@dsnet This bug points out two important documentation fixes that should be made. Since you seem in a hurry to close it, would you mind opening a new bug to note the needed doc fixes to time.Round and time.Truncate? Both claim they are unchanged with a zero argument, but that is no longer true in 1.9.

@dsnet dsnet reopened this Aug 17, 2017
@dmitshur dmitshur changed the title time: go1.9rc2 stringifies time.Time in a backwards incompatible way, adding m=+0.000241395 time: Round(0), Truncate(0) strip monotonic clock readings but documentation still says it returns t unchanged Aug 17, 2017
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 17, 2017
@dmitshur
Copy link
Contributor

I'll mention https://talks.godoc.org/github.com/davecheney/go-1.9-release-party/presentation.slide#26 here too, since it's relevant to the topic in the original issue. /cc @davecheney

@gopherbot
Copy link

Change https://golang.org/cl/56690 mentions this issue: time: fix documentation of Round, Truncate behavior for d <= 0

@glycerine
Copy link
Author

@shurcooL the CL looks nice, thanks for doing it. It is probably worth reiterating one additional note in the time.Round doc as well, since it is buried in the wall of text up front and I missed it on first pass:

The canonical way to strip a monotonic clock reading is to use t = t.Round(0).

@dmitshur
Copy link
Contributor

@glycerine Ive considered doing that, but saw that it wouldn't be helpful. If the user doesn't already know Round(0) is the canonical way to strip mono, they won't be able to jump to its documentation to learn that. It's better to leave the rest of time package docs to say Round(0) is canonical. Other ways are valid too.

@glycerine
Copy link
Author

glycerine commented Aug 18, 2017

What will happen is this: user sees call to Round(0) in some code they haven't written themselves. Question occurs to user: what the heck is this Round(0) call doing? It doesn't make any sense!?! Let's look at the documentation for Round! User positions cursor over the Round(0) call and presses F12 (or whatever godef is bound to), and jumps to the documentation for Round. Now does the user get a clue? They could, if we help them out here. Or we could just leave them mystified.

I'm not saying remove anything. I'm just saying we should add to the Round documentation that one sentence to explain Round(0); in addition to the preamble stuff.

Come on. Let's not be stingy with docs. This is crappy API, as rsc agreed. Let's help all we can.

@dmitshur
Copy link
Contributor

what the heck is this Round(0) call doing? ... jumps to the documentation for Round. Now does the user get a clue?

Yes, the docs will say:

If d <= 0, Round returns t stripped of any monotonic clock reading but otherwise unchanged.

So the user will know that Round(0) was used to strip a monotonic clock reading.

@aclements aclements modified the milestones: Go1.9, Go1.9.1 Aug 25, 2017
@dsnet
Copy link
Member

dsnet commented Sep 5, 2017

Re-opening for cherry-pick.

@dsnet dsnet reopened this Sep 5, 2017
@rsc rsc modified the milestones: Go1.9.1, Go1.9.2 Oct 4, 2017
@ianlancetaylor ianlancetaylor added release-blocker and removed NeedsFix The path to resolution is known, but the work has not been done. labels Oct 13, 2017
@rsc
Copy link
Contributor

rsc commented Oct 13, 2017

CL 56690 OK for Go 1.9.2 (doc fix).

@rsc rsc added the CherryPickApproved Used during the release process for point releases label Oct 14, 2017
@gopherbot
Copy link

Change https://golang.org/cl/70846 mentions this issue: [release-branch.go1.9] time: fix documentation of Round, Truncate behavior for d <= 0

gopherbot pushed a commit that referenced this issue Oct 25, 2017
…avior for d <= 0

Saying that they return t unchanged is misleading, because they return
a modified t, stripped of any monotonic clock reading, as of Go 1.9.

Fixes #21485.

Change-Id: Icddf8813aed3d687fcefcd2fe542829438be6a0a
Reviewed-on: https://go-review.googlesource.com/56690
Reviewed-by: Avelino <t@avelino.xxx>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/70846
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Russ Cox <rsc@golang.org>
@rsc
Copy link
Contributor

rsc commented Oct 26, 2017

go1.9.2 has been packaged and includes:

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Oct 26 21:09:07 UTC

@rsc rsc closed this as completed Oct 26, 2017
@golang golang locked and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases Documentation FrozenDueToAge release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants