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: MarshalBinary fails on valid Time #39616

Open
kayleg opened this issue Jun 16, 2020 · 15 comments · May be fixed by #40293
Open

time: MarshalBinary fails on valid Time #39616

kayleg opened this issue Jun 16, 2020 · 15 comments · May be fixed by #40293

Comments

@kayleg
Copy link

@kayleg kayleg commented Jun 16, 2020

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

$ go version
go version go1.14.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/path/Library/Caches/go-build"
GOENV="/Users/path/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOOS="darwin"
GOPATH="/Users/path/code/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
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"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/bf/vxlr89hd4jqfx43ncc0y10b00000gn/T/go-build493699043=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Call In on a zero value time.Time creates in valid time when the result would be before the zero time.

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

What did you expect to see?

The zero value time would have its time zone set to the specified timezone yet still be the zero value date.

For Example:
Setting 0001-01-01 00:00 UTC to EDT should return 0001-01-01 00:00 EDT or return an error if the value is outside the range (which in this case it is)

Using zeroTime.After(newTime) should return true or some other mechanism to check the time is valid should exist.

What did you see instead?

A time value before the range was returned with an invalid time zone 0000-12-31 19:03:58 -0456 LMT.

Attempting to compare the result against the zero time using After does not function as zeroTime.After(newTime) and newTime.After(zeroTime) both return false so you can have a false positive that the time is valid.

Also the resulting time value prevents MarshalBinary from working

@davecheney
Copy link
Contributor

@davecheney davecheney commented Jun 17, 2020

Thank you for raising this issue. Before we can investigate could you please update your sample code to check all errors and verify that the issues is still present.

@kayleg
Copy link
Author

@kayleg kayleg commented Jun 17, 2020

I updated the sample code with error checks and better output. The problem still occurs.

@davecheney
Copy link
Contributor

@davecheney davecheney commented Jun 17, 2020

0001 ad is not within the range that time.Time supports, please try https://play.golang.org/p/7JicBQHKeeq

@kayleg
Copy link
Author

@kayleg kayleg commented Jun 17, 2020

Please point me to the documentation that states what is the minimum supported value.

According to the documentation for time.Parse: The year value must be between 0000 and 9999. And non specified values are interpreted as 0 values.

So for this example using time.Kitchen format:
https://play.golang.org/p/M55U4VgvuNI
You can still end up with an invalid time:

Kitchen UTC: 0000-01-01 12:00:00 +0000 UTC
Kitchen Eastern: 0000-01-01 07:03:58 -0456 LMT

The cut off between invalid and valid times seems to be at: 1883-11-18T16:59:59Z
https://play.golang.org/p/Fe44XV7yIMs

@davecheney
Copy link
Contributor

@davecheney davecheney commented Jun 17, 2020

Thank you for correcting me. I agree that parse suggests it accepts a 10000 year range. The closest I can find to the range of values that parse can return comes from https://golang.org/pkg/time/#Time.UnixNano. I cannot seem to find any documentation other than this.

I’m having trouble understanding your original question. This is my fault, I’m just not understanding what you are saying. Is it possible for you to restate your problem in the form of a goal “I need to x, do to x I tried to y”?

@kayleg
Copy link
Author

@kayleg kayleg commented Jun 17, 2020

Sure. No problem.

I have 2 use cases:

  1. I need to convert a UTC timestamp from a database to Local time and encode the value using gob. I tried using utcValue.In(localLocation)
  2. I need to localize a UTC time in the Kitchen format and encode the value using gob. I tried utcValue.In(localLocation)

For both cases local timezone is US/Eastern

@ianlancetaylor ianlancetaylor changed the title Calling time.In on a zero value time can produce a non valid time value time: MarshalBinary fails on valid Time Jun 17, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 17, 2020

If I'm reading this right, the basic problem is that MarshalBinary is failing when it shouldn't. Here is a simpler example that doesn't rely on the zero time.

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

package main

import (
	"fmt"
	"time"
)

func main() {
	t, err := time.Parse(time.RFC3339, "1880-01-01T00:00:00Z")
	if err != nil {
		fmt.Println("Failed to parse time", err)
	}
	loc, err := time.LoadLocation("US/Eastern")
	if err != nil {
		fmt.Println("Failed to load location", err)
	}
	t2 := t.In(loc)
	fmt.Println("Zero Eastern:", t2)
	b, err := t2.MarshalBinary()
	if err != nil {
		fmt.Println("Failed to marshal", err)
	} else {
		fmt.Println("Got bytes", b)
	}
}
@kayleg
Copy link
Author

@kayleg kayleg commented Jun 17, 2020

Yes, the main crux is MarshalBinary doesn't support local mean time at the moment.

Some Info:

Part of the problem is with changing the time zone for values before time zones existed.
If you look at the output of t2 you will see the minutes and seconds are no longer 0 and the timezone becomes some fractional local mean time.

Which looks to be based on when US railways adopted time zones: https://en.wikipedia.org/wiki/Time_zone#cite_ref-8

(Adding the above to the documentation for In would help reduce confusion especially for people not versed in US railway history 😄 )

I guess the question of: Should we maintain historic accuracy or expected behavior?
If the user asks for US/Eastern then returning LMT is confusing without any error.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 18, 2020

We return LMT because that is what the timezone database tells us to do.

@kayleg
Copy link
Author

@kayleg kayleg commented Jun 18, 2020

Makes sense - that's what I expected is the case.

@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Jul 2, 2020

So should I implement LMT in MarshalBinary to solve this problem?

@kayleg
Copy link
Author

@kayleg kayleg commented Jul 2, 2020

That would fix my use case.

@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Jul 5, 2020

I am currently working on this issue. As what I asked, I was refactoring MarshalBinary() to support LMT.
However, I met a problem that LMT may have fractional minute. To support the current binary format of the output of MarshalBinary() the fractional minute part would be ignored. Or should we modify the format of the output of MarshalBinary() and add two more bytes to support LMT?
@ianlancetaylor any suggestion?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 5, 2020

I would be OK with using timeBinaryVerison 2 to represent timezone seconds when required. We should still use version in the normal case.
Thanks.

HowJMay added a commit to HowJMay/go that referenced this issue Jul 19, 2020
If the time is in 'LMT' and has fractional minute, then
`MarshalBinary()` and `UnmarshalBinary()` will encode/decode the time
in `timeBinaryVersionV2` in which the fractional minute is at
bit 15 and 16, and presented in seconds.

Fixes golang#39616
HowJMay added a commit to HowJMay/go that referenced this issue Jul 19, 2020
If the time is in 'LMT' and has fractional minute, then
`MarshalBinary()` and `UnmarshalBinary()` will encode/decode the time
in `timeBinaryVersionV2` in which the fractional minute is at
bit 15 and 16, and presented in seconds.

Fixes golang#39616
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 19, 2020

Change https://golang.org/cl/243402 mentions this issue: src/time: Support LMT in MarshalBinary, UnmarshalBinary

HowJMay added a commit to HowJMay/go that referenced this issue Jul 20, 2020
If the time is in 'LMT' and has fractional minute, then
`MarshalBinary()` and `UnmarshalBinary()` will encode/decode the time
in `timeBinaryVersionV2` in which the fractional minute is at
bit 15 and 16, and presented in seconds.

Fixes golang#39616
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.