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: Now() and time.unixTime() do not call setLoc, causing timezone to not be nil when system time is UTC #19486

Closed
sbuss opened this issue Mar 10, 2017 · 21 comments

Comments

Projects
None yet
6 participants
@sbuss
Copy link

commented Mar 10, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version devel +4cce27a3fa Sat Jan 21 03:20:55 2017 +0000 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/usr/local/google/home/sbuss/workspace"
GORACE=""
GOROOT="/usr/lib/google-golang"
GOTOOLDIR="/usr/lib/google-golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build194178423=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

When running go code in a system set to UTC, or by setting time.Local = time.UTC, golang does not correctly set loc to nil when either time.Now() or time.Unix() are called. You can reproduce the issue via this play link.

What did you expect to see?

I expected the timestamps generated by Now() to have loc = nil, as stated by the docs on time.Time

What did you see instead?

The timestamps actually have this weird Location: &{UTC [] [] %!s(int64=0) %!s(int64=0) %!s(*time.zone=<nil>)}

@gopherbot

This comment has been minimized.

Copy link

commented Mar 10, 2017

CL https://golang.org/cl/38002 mentions this issue.

@cespare

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2017

@sbuss note that the "docs" you linked to are comments on an unexported field of time.Time, not part of the public documentation of the type.


I've also noticed this in the following context: if you have a test that creates a timestamp with time.Now(), round-trips it through, say, JSON, and then compares the result to the original timestamp using == (or more commonly via reflect.DeepEquals where the timestamp is buried in some other structure), the test will pass unless the local tz offset is +0 (because the non-nil location round-trips to nil).

In this case the fix is to not write tests that use == on time.Times. But it is an interesting gotcha.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2017

I'm not persuaded. I don't think the time package needs to cater to people who explicitly set time.Local = time.UTC. And I don't think the time package makes any promises about the unexported loc field for the case where the local timezone happens to be UTC.

If we're going to make this change, it needs a better rationale than I see so far in this description. For example, it needs an explanation in terms of something else that matters and can be clearly understood, like #15716.

@sbuss

This comment has been minimized.

Copy link
Author

commented Mar 10, 2017

I don't think the time package needs to cater to people who explicitly set time.Local = time.UTC

I included time.Local = time.UTC in the play link because I thought it'd be better to be explicit. But this also happens if your system timezone is set to UTC, like the golang playground: https://play.golang.org/p/sP141RpCuF

I've also noticed this in the following context: if you have a test that creates a timestamp with time.Now(), round-trips it through, say, JSON, and then compares the result to the original timestamp using == (or more commonly via reflect.DeepEquals where the timestamp is buried in some other structure), the test will pass unless the local tz offset is +0 (because the non-nil location round-trips to nil).

This is exactly how I discovered this issue, in fact. I was storing and retrieving some data to Google Cloud Datastore and doing a reflect.DeepEqual on the before-and-after. This manifests as the time you put into datastore not equaling the time you get from datastore. I know that users are supposed to use time.Equal, and we've since updated our tests, but the simple patch in the linked CL does fix some surprising behavior.

@cespare

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2017

@ianlancetaylor well, more or less the same justification as #15716 applies here (it could be phrased as "Time.MarshalJSON and Time.UnmarshalJSON aren't symmetrical if the local timezone is UTC"):

package main

import (
	"fmt"
	"log"
	"time"
)

func main() {
	t0 := time.Now()
	s := t0.Format(time.RFC3339Nano)
	t1, err := time.Parse(time.RFC3339Nano, s)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Printf("t0==t1: %t\n", t0 == t1) // true, unless the location is UTC
	fmt.Printf("t0: %#v\n", t0)
	fmt.Printf("t1: %#v\n", t1)
}

https://play.golang.org/p/xx0fU4gn45

The commit message for 5fbf35d that fixed #15716 is relevant.

/cc @rsc

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2017

@cespare Thanks, I'm fine with fixing that problem, but then let's update the CL to be about that problem, with an appropriate test, rather than a test that explicitly sets Local = UTC.

@sbuss

This comment has been minimized.

Copy link
Author

commented Mar 10, 2017

but then let's update the CL to be about that problem, with an appropriate test, rather than a test that explicitly sets Local = UTC.

SGTM I will do this tomorrow

@sbuss

This comment has been minimized.

Copy link
Author

commented Mar 10, 2017

While trying to write the test you recommended, I discovered another potential bug in the time marshaling functions (#19502). I'll wait until learning more in that bug before revising my patch for this issue.

Edit: I figured out that that was intended behavior and know how to do this correctly. Should have a new patch soon.

@sbuss

This comment has been minimized.

Copy link
Author

commented Mar 10, 2017

I can't write a test which uses MarshalXYZ or UnmarshalXYZ because I have to disable the monotonic clock via AddDate(0, 0, 0), which calls setLoc and effectively masks the bug. So I can't meet your ask exactly, @ianlancetaylor, but the code snippet from @cespare is close enough that I'll go with that.

@sbuss

This comment has been minimized.

Copy link
Author

commented Mar 10, 2017

Actually, I can't use @cespare's code snippet, either, because of the same issue with the monotonic clock. You'll always get different structs if the monotonic clock isn't disabled, but the act of disabling it hides the bug. @ianlancetaylor Is there something else I could do to the patch or tests to make you more in favor of it?

Note that this issue doesn't show up on the golang playground yet because it's running a version of go that doesn't include the monotonic clock.

@cespare

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2017

If we consider the test example, in Go 1.9 it will fail in all timezones; there's nothing special about UTC. So the motivation here kind of goes away. Perhaps we should just leave it alone.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2017

I believe you can use t.Round(0) to get the same value without a monotonic time and without calling setLoc.

@sbuss

This comment has been minimized.

Copy link
Author

commented Mar 13, 2017

I believe you can use t.Round(0) to get the same value without a monotonic time and without calling setLoc.

Yup, that did it. Thanks for the pointer!

I just posted an updated patch which tests round trips through the Marshal/Unmarshal functions explicitly. Without the fix to Now and unixTime, the new test fails like this:

--- FAIL: TestDefaultLocNow (0.00s)
        time_test.go:1275: Now() and Now().UTC() behave differently for MarshalBinaryRoundTrip
        time_test.go:1275: Now() and Now().UTC() behave differently for GobEncodeRoundTrip
        time_test.go:1275: Now() and Now().UTC() behave differently for MarshalJSONRoundTrip
        time_test.go:1275: Now() and Now().UTC() behave differently for MarshalTextRoundTrip
        time_test.go:1275: Unix(...) and Unix(...).UTC() behave differently for MarshalBinaryRoundTrip
        time_test.go:1275: Unix(...) and Unix(...).UTC() behave differently for GobEncodeRoundTrip
        time_test.go:1275: Unix(...) and Unix(...).UTC() behave differently for MarshalJSONRoundTrip
        time_test.go:1275: Unix(...) and Unix(...).UTC() behave differently for MarshalTextRoundTrip

rather than a test that explicitly sets Local = UTC

I don't know how I'd affect the local timezone of the test runner without explicitly setting Local = UTC. Do you have a suggested way of doing this?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2017

For testing purposes I suspect we need a new function like initTestingZone that sets the local location name to "UTC", just as initLocal does.

Seems like another way to fix this problem is to change MarshalJSON and friends to not use RFC3339Nano but instead use "2006-01-02T15:04:05.999999999-0700".

@bradfitz bradfitz changed the title time.Now() and time.unixTime() do not call setLoc, causing timezone to not be nil when system time is UTC time: Now() and time.unixTime() do not call setLoc, causing timezone to not be nil when system time is UTC Mar 21, 2017

@bradfitz bradfitz added this to the Go1.9Maybe milestone Mar 21, 2017

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

I do not believe we support packages overwriting package time's locations. That is, clients are not expected to do:

time.Local = time.UTC

That's not OK. It may cause arbitrary breakage (as it does here), and that breakage may change from release to release.

@rsc rsc closed this Apr 7, 2017

@cespare

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

@rsc setting time.Local = time.UTC is a red herring; it was just how @sbuss was demonstrating the issue.

If I run this code (1.8 or tip):

package main

import (
	"encoding/json"
	"fmt"
	"log"
	"time"
)

func main() {
	t0 := time.Now().Round(0)
	b, err := json.Marshal(t0)
	if err != nil {
		log.Fatal(err)
	}

	var t1 time.Time
	if err := json.Unmarshal(b, &t1); err != nil {
		log.Fatal(err)
	}

	fmt.Printf("t0 == t1: %t\n", t0 == t1)
	fmt.Printf("t0.Equal(t1): %t\n", t0.Equal(t1))
	fmt.Printf("t0: %#v\n", t0)
	fmt.Printf("t1: %#v\n", t1)
}

on my local machine in PDT, I get:

t0 == t1: true
t0.Equal(t1): true
t0: time.Time{wall:0x370fff54, ext:63627184829, loc:(*time.Location)(0x586220)}
t1: time.Time{wall:0x370fff54, ext:63627184829, loc:(*time.Location)(0x586220)}

and if I run on a server in UTC, I get

t0 == t1: false
t0.Equal(t1): true
t0: time.Time{wall:0x2d79e290, ext:63627184833, loc:(*time.Location)(0x586220)}
t1: time.Time{wall:0x2d79e290, ext:63627184833, loc:(*time.Location)(nil)}

So the issue is that we got a timestamp that round-trips through JSON in PDT but not in UTC. (The same applies to other encodings as @sbuss's patch shows.)

Maybe we don't care about people using == to compare time.Times (commonly done by accident using reflect.DeepEqual), but your change 5fbf35d seems to indicate that we do care a little bit.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

@cespare Do you know of any reason we can't change MarshalJSON as I suggested above?

@cespare

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

@ianlancetaylor that might be okay, though I suppose on the margin it could break (strange) code that expects JSON-encoded UTC timestamps to use Z. Wouldn't we need to do similar fixes for the other encodings too? (MarshalText MarshalBinary/gob).

I guess my concern is that 5fbf35d purports to introduce the invariant that the UTC time.Times are only ever represented using a nil loc:

In the zero Time, the (not user visible) nil *Location indicates UTC.
In the result of t.UTC() and other ways to create times in specific
zones, UTC is indicated by a non-nil *Location, specifically &utcLoc.
This creates a representation ambiguity exposed by comparison with ==
or reflect.DeepEqual or the like.

Change time.Time representation to use only nil, never &utcLoc,
to represent UTC. This eliminates the ambiguity.

This bug is highlighting two cases -- time.Now and time.Unix -- where this doesn't hold. Now this invariant isn't part of the public API, but it seems to me that the reasoning given before should apply in these cases as well.

From this perspective, changing the JSON and other encodings seems to be papering over an underlying ambiguity.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

I don't believe we can change MarshalJSON at this point, and I don't believe there is an actual problem here. All time formattings are lossy. It may well be that t0!=t1 but after a round trip serialization they are equal. For example if they differ only in the final nanosecond bit but the time only records microseconds, non-equal times become equal. The same thing (but different) is happening here. JSON cannot distinguish "UTC 00:00" from "Local 00:00" the way Go does.

This is a dup of #17875 / #17885.

@cespare

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2017

@rsc Since you added setLoc to Parse, Time.UTC, Time.Local, Time.In, and Time.UnmarshalBinary in 5fbf35d to preserve the invariant that UTC Times have a nil loc, I don't understand why the same reasoning doesn't apply to Now and Unix (this issue). But I'll stop pursuing this now.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

Change 5fbf35d was about making sure that the internal location pointer in a time.Time has a consistent internal representation for UTC times, namely nil and not &utcLoc.

The problem in the program in #19486 (comment) is different. Go always maintains a distinction between local and UTC times, even when TZ=UTC in the environment. Given that, the problem is that JSON encoding does not preserve this distinction, so it can't round trip back to the same representation:

$ cat /tmp/x.go
package main

import (
	"encoding/json"
	"fmt"
	"time"
)

func main() {
	t := time.Unix(1e9, 0)
	fmt.Printf("local %s\n", js(t.Local()))
	fmt.Printf("utc   %s\n", js(t.UTC()))
}

func js(x interface{}) string {
	js, _ := json.Marshal(x)
	return string(js)
}
$ go run /tmp/x.go
local "2001-09-08T21:46:40-04:00"
utc   "2001-09-09T01:46:40Z"
$ TZ=UTC go run /tmp/x.go
local "2001-09-09T01:46:40Z"
utc   "2001-09-09T01:46:40Z"
$ 

There's no way JSON unmarshaling can decode that pair of strings back to two different time.Times. One must be mapped to the other.

@lkysow lkysow referenced this issue Jul 11, 2017

Merged

Fix test #74

@golang golang locked and limited conversation to collaborators Apr 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.