Skip to content

time: time zone lookup using extend string makes wrong start time for non-DST zones #58682

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.

Sign up for GitHub

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
Closed
KimMachineGun opened this issue Feb 24, 2023 · 8 comments
Closed

time: time zone lookup using extend string makes wrong start time for non-DST zones #58682

KimMachineGun opened this issue Feb 24, 2023 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@KimMachineGun
Copy link
Contributor

KimMachineGun commented Feb 24, 2023 •
edited
Loading

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

$ go version
go version go1.20 darwin/arm64

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="arm64"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOOS="darwin"
GOVERSION="go1.20"

What did you do?

package main

import (
	"fmt"
	"time"
)

func main() {
	loc, err := time.LoadLocation("Asia/Seoul")
	if err != nil {
		panic(err)
	}

	fmt.Println(time.Date(2023, 2, 24, 0, 0, 0, 0, loc).ZoneBounds())
}

What did you expect to see?

Since the last transition in Asia/Seoul on Sunday, 9 October 1988, 02:00:00, the expected result:

1988-10-09 02:00:00 +0900 KST 0001-01-01 00:00:00 +0000 UTC

What did you see instead?

292277026596-12-05 00:30:07 +0900 KST 0001-01-01 00:00:00 +0000 UTC

Debugging

It seems like time.LoadLocationFromTZData and time.Location.lookup work inappropriately if the time is after the last transition and the extend string doesn't have DST rule.
I think the problematic parts are these:

// `time.LoadLocationFromTZData` (`time.Location.lookup` is similar to this)

// Fill in the cache with information about right now,
// since that will be the most common lookup.
sec, _, _ := now()
for i := range tx {
	if tx[i].when <= sec && (i+1 == len(tx) || sec < tx[i+1].when) {
		l.cacheStart = tx[i].when
		l.cacheEnd = omega
		l.cacheZone = &l.zone[tx[i].index]
		if i+1 < len(tx) {
			l.cacheEnd = tx[i+1].when
		} else if l.extend != "" {
			// If we're at the end of the known zone transitions,
			// try the extend string.
			if name, offset, estart, eend, isDST, ok := tzset(l.extend, l.cacheEnd, sec); ok {
				l.cacheStart = estart
				l.cacheEnd = eend
				// Find the zone that is returned by tzset to avoid allocation if possible.
				if zoneIdx := findZone(l.zone, name, offset, isDST); zoneIdx != -1 {
					l.cacheZone = &l.zone[zoneIdx]
				} else {
					l.cacheZone = &zone{
						name:   name,
						offset: offset,
						isDST:  isDST,
					}
				}
			}
		}
		break
	}
}
// func tzset(s string, initEnd, sec int64) (name string, offset int, start, end int64, isDST, ok bool) {

if len(s) == 0 || s[0] == ',' {
	// No daylight savings time.
	return stdName, stdOffset, initEnd, omega, false, true
}

Based on tzfile(5) manual, the extend string is applied to the last transition, but the above codes pass the last transition's end time(omega), and tzset uses it as a start time in non-DST case.

It leads us to two problems:

  • time.Time.ZoneBounds method returns wrong results.
  • zone lookup always needs binary search to find a zone for the time after the last transition because the cacheStart is omega.

Due to the second problem, some operations that need timezone information for non-DST fall into significant performance degradation.

This is a benchstat result after solving the problem for some cases. (in Asia/Seoul)

goos: darwin
goarch: arm64
pkg: time
                        │ docs/benchmark/old.txt │       docs/benchmark/new.txt        │
                        │         sec/op         │   sec/op     vs base                │
Now-10                               40.49n ± 0%   39.79n ± 0%   -1.72% (p=0.000 n=10)
NowUnixNano-10                       38.44n ± 1%   38.25n ± 0%        ~ (p=0.867 n=10)
NowUnixMilli-10                      38.05n ± 0%   38.10n ± 0%        ~ (p=0.180 n=10)
NowUnixMicro-10                      38.07n ± 1%   38.06n ± 1%        ~ (p=0.753 n=10)
Format-10                            126.8n ± 1%   110.0n ± 0%  -13.25% (p=0.000 n=10)
FormatRFC3339-10                     59.31n ± 0%   45.70n ± 0%  -22.94% (p=0.000 n=10)
FormatRFC3339Nano-10                 60.55n ± 0%   46.80n ± 0%  -22.72% (p=0.000 n=10)
FormatNow-10                         128.5n ± 0%   111.9n ± 0%  -12.96% (p=0.000 n=10)
MarshalJSON-10                       77.21n ± 1%   62.32n ± 0%  -19.29% (p=0.000 n=10)
MarshalText-10                       77.43n ± 0%   62.04n ± 1%  -19.87% (p=0.000 n=10)
Parse-10                             113.7n ± 0%   113.6n ± 0%        ~ (p=0.197 n=10)
ParseRFC3339UTC-10                   43.85n ± 0%   43.81n ± 0%        ~ (p=0.809 n=10)
ParseRFC3339UTCBytes-10              45.65n ± 1%   45.65n ± 0%        ~ (p=0.539 n=10)
ParseRFC3339TZ-10                    70.88n ± 0%   55.53n ± 0%  -21.65% (p=0.000 n=10)
ParseRFC3339TZBytes-10               84.36n ± 0%   69.38n ± 1%  -17.76% (p=0.000 n=10)
ParseDuration-10                     62.66n ± 2%   62.62n ± 0%        ~ (p=0.183 n=10)
Hour-10                             19.245n ± 0%   3.397n ± 0%  -82.35% (p=0.000 n=10)
Second-10                           19.230n ± 0%   3.405n ± 2%  -82.29% (p=0.000 n=10)
Year-10                             21.740n ± 0%   7.315n ± 1%  -66.35% (p=0.000 n=10)
Day-10                              22.695n ± 2%   8.938n ± 0%  -60.62% (p=0.000 n=10)
ISOWeek-10                           24.33n ± 0%   10.76n ± 0%  -55.77% (p=0.000 n=10)
GoString-10                          93.85n ± 1%   76.47n ± 1%  -18.52% (p=0.000 n=10)
UnmarshalText-10                     68.17n ± 1%   52.78n ± 0%  -22.58% (p=0.000 n=10)
geomean                              51.04n        35.33n       -30.78%

                        │ docs/benchmark/old.txt │       docs/benchmark/new.txt        │
                        │          B/op          │    B/op     vs base                 │
Now-10                              0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
NowUnixNano-10                      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
NowUnixMilli-10                     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
NowUnixMicro-10                     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Format-10                           24.00 ± 0%     24.00 ± 0%       ~ (p=1.000 n=10) ¹
FormatRFC3339-10                    32.00 ± 0%     32.00 ± 0%       ~ (p=1.000 n=10) ¹
FormatRFC3339Nano-10                32.00 ± 0%     32.00 ± 0%       ~ (p=1.000 n=10) ¹
FormatNow-10                        32.00 ± 0%     32.00 ± 0%       ~ (p=1.000 n=10) ¹
MarshalJSON-10                      48.00 ± 0%     48.00 ± 0%       ~ (p=1.000 n=10) ¹
MarshalText-10                      48.00 ± 0%     48.00 ± 0%       ~ (p=1.000 n=10) ¹
Parse-10                            0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseRFC3339UTC-10                  0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseRFC3339UTCBytes-10             0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseRFC3339TZ-10                   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseRFC3339TZBytes-10              48.00 ± 0%     48.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseDuration-10                    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Hour-10                             0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Second-10                           0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Year-10                             0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Day-10                              0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ISOWeek-10                          0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
GoString-10                         80.00 ± 0%     80.00 ± 0%       ~ (p=1.000 n=10) ¹
UnmarshalText-10                    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                        ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                        │ docs/benchmark/old.txt │       docs/benchmark/new.txt        │
                        │       allocs/op        │ allocs/op   vs base                 │
Now-10                              0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
NowUnixNano-10                      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
NowUnixMilli-10                     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
NowUnixMicro-10                     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Format-10                           1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
FormatRFC3339-10                    1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
FormatRFC3339Nano-10                1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
FormatNow-10                        1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
MarshalJSON-10                      1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
MarshalText-10                      1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
Parse-10                            0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseRFC3339UTC-10                  0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseRFC3339UTCBytes-10             0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseRFC3339TZ-10                   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseRFC3339TZBytes-10              1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseDuration-10                    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Hour-10                             0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Second-10                           0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Year-10                             0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Day-10                              0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
ISOWeek-10                          0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
GoString-10                         1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
UnmarshalText-10                    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                        ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean
KimMachineGun added a commit to KimMachineGun/go that referenced this issue Feb 24, 2023
@KimMachineGun
fix(time): fix timezone lookup login for non-DST zones
b70b16d 
Fixes golang#58682
@KimMachineGun KimMachineGun mentioned this issue Feb 24, 2023
Closed
KimMachineGun added a commit to KimMachineGun/go that referenced this issue Feb 24, 2023
@KimMachineGun
fix(time): fix timezone lookup logic for non-DST zones
c5c9b72 
Fixes golang#58682
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/471020 mentions this issue: fix(time): fix timezone lookup logic for non-DST zones

KimMachineGun added a commit to KimMachineGun/go that referenced this issue Feb 24, 2023
@KimMachineGun
time: fix timezone lookup logic for non-DST zones
54b2015 
Fixes golang#58682
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 24, 2023
@thanm
Copy link
Contributor

thanm commented Feb 24, 2023

@rsc per owners.

@KimMachineGun
Copy link
Contributor Author

KimMachineGun commented Mar 9, 2023 •
edited
Loading

@thanm @rsc
Please let me know if I can do anything to help progress the investigation process.

@KimMachineGun
Copy link
Contributor Author

KimMachineGun commented Mar 16, 2023 •
edited
Loading

@ianlancetaylor
This bug has been exposed since 1.19. How about backporting this fix?

@ianlancetaylor
Copy link
Contributor

@gopherbot Please open backport issues

We incorrectly handle the timezone bounds for timezones that used to have daylight savings time and then stopped.

This was referenced Mar 16, 2023
Closed
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #59074 (for 1.19), #59075 (for 1.20).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/478657 mentions this issue: [release-branch.go1.19] time: fix timezone lookup logic for non-DST zones

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/478658 mentions this issue: [release-branch.go1.20] time: fix timezone lookup logic for non-DST zones

gopherbot pushed a commit that referenced this issue Mar 23, 2023
@KimMachineGun @gopherbot
[release-branch.go1.20] time: fix timezone lookup logic for non-DST z…
65fa8a6 
…ones

This change fixes time.LoadLocationFromTZData and time.Location.lookup logic if the given time is after the last transition and the extend string doesn't have the DST rule.

For #58682
Fixes #59075

Change-Id: Ie34a6d658d14c2b33098b29ab83c041ef0d34266
GitHub-Last-Rev: f6681eb
GitHub-Pull-Request: #58684
Reviewed-on: https://go-review.googlesource.com/c/go/+/471020
Reviewed-by: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
(cherry picked from commit 90dde5d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/478658
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>
Auto-Submit: Heschi Kreinick <[email protected]>
gopherbot pushed a commit that referenced this issue Mar 23, 2023
@KimMachineGun @gopherbot
[release-branch.go1.19] time: fix timezone lookup logic for non-DST z…
2adec52 
…ones

This change fixes time.LoadLocationFromTZData and time.Location.lookup logic if the given time is after the last transition and the extend string doesn't have the DST rule.

For #58682
Fixes #59074

Change-Id: Ie34a6d658d14c2b33098b29ab83c041ef0d34266
GitHub-Last-Rev: f6681eb
GitHub-Pull-Request: #58684
Reviewed-on: https://go-review.googlesource.com/c/go/+/471020
Reviewed-by: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
Run-TryBot: Ian Lance Taylor <[email protected]>
(cherry picked from commit 90dde5d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/478657
Reviewed-by: Heschi Kreinick <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Heschi Kreinick <[email protected]>
@golang golang locked and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

time: fix timezone lookup logic for non-DST zones KimMachineGun/go
4 participants
@ianlancetaylor @gopherbot @thanm @KimMachineGun