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

Jump to bottom

time.UTC() changes the value of time.Now().UTC().Round(time.Millisecond) #43630

Closed
tatatodd opened this issue Jan 11, 2021 · 6 comments
Closed

time.UTC() changes the value of time.Now().UTC().Round(time.Millisecond) #43630

tatatodd opened this issue Jan 11, 2021 · 6 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@tatatodd
Copy link
Contributor

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

$ go version
go version go1.15.6 darwin/amd64

Does this issue reproduce with the latest release?

Yes, go1.15.6 is the latest release

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/toddwang/Library/Caches/go-build"
GOENV="/Users/toddwang/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/toddwang/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/toddwang/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/toddwang/src/neeva/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/rw/lyr53c8115bf5y6q_4f0_yyh0000gn/T/go-build292212815=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have code and unit tests for my real system that boils down to this:
https://play.golang.org/p/4afKq3-_kHI

	now := time.Now().UTC().Round(time.Millisecond)
	t2 := now.UTC()
	fmt.Println(now == t2)     // (1) This sometimes returns false, unexpected.
	fmt.Println(now.Equal(t2)) // (2) This seems to always returns true, as expected.

My "real world unittest" fails like this (expected=now, actual=t2):

        	Error:      	Not equal: 
        	            	expected: time.Time{wall:0x256e8500, ext:63745942249, loc:(*time.Location)(nil)}
        	            	actual  : time.Time{wall:0x0, ext:63745942250, loc:(*time.Location)(nil)}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,4 +1,4 @@
        	            	 (time.Time) {
        	            	- wall: (uint64) 628000000,
        	            	- ext: (int64) 63745942249,
        	            	+ wall: (uint64) 0,
        	            	+ ext: (int64) 63745942250,
        	            	  loc: (*time.Location)(<nil>)

What did you expect to see?

I expected that now == t2 would always return true. I realize this may be arguable. Perhaps that's where I went wrong?

Specifically, I expected that calling now.UTC() in my example would be a no-op, since now is already in UTC.

What did you see instead?

I see now != t2 occasionally, which I didn't expect.
@AlexRouSg
Copy link
Contributor

Have you tried running go test -race to check for race conditions? The fact that it only happens sometimes seems to indicate a data race somewhere or your test is not doing what you think it is doing..

@tatatodd
Copy link
Contributor Author

The test is synchronous. I'm assuming a different explanation for why this only happens sometimes, is because it's dependent on the actual time values stored in wall and ext. I haven't had time to trace through the go time.Time code yet, but that sounds plausible to me.

@ianlancetaylor
Copy link
Contributor

As documented at https://golang.org/pkg/time/#Time, using == with time.Time values requires careful attention. Is there a reason you don't just use Time.Equal?

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 11, 2021
@tatatodd
Copy link
Contributor Author

Actually we just had a unittest failure for the above logic, that uses Time.Equal.

I haven't dug through all the code, but I'm suspecting that time.Round(time.Millisecond) doesn't always strip the monotonic clock? I.e. Round strips the monotonic clock, but then calls Add, which seems like it might revive it:
https://golang.org/src/time/time.go?s=43793:43829#L1410
https://golang.org/src/time/time.go?s=26512:26546#L803

And I believe Time.Round is documented to always strip the monotonic clock. But again, I haven't actually dug all the way through.

@ianlancetaylor
Copy link
Contributor

Time.Round always strips the monotonic clock. Time.Add only adjusts the monotonic clock if it is present.

If you think there is a bug here I think you need to show us a complete, standalone, test case. Thanks.

@tatatodd
Copy link
Contributor Author

Thanks for your help, @ianlancetaylor and @AlexRouSg. It turns out it was a problem in our code all along, and I was just confused by the Go internal time representation. Closing this out.

@golang golang locked and limited conversation to collaborators Jan 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ianlancetaylor @tatatodd @AlexRouSg @gopherbot