time: LoadLocation returns undescriptive error messages if no time zone database is available

[GreenLightning]

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

go version go1.17 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system are you using?

Windows

What did you do?

Attempt to load time zone information using time.LoadLocation() and test under various conditions where no time zone database is available.

This issue is made worse by the fact that the Go distribution includes a time zone database, but it is not embedded into programs being compiled by default. Therefore binaries of my program worked fine for me, but not for my users that did not have Go installed. The "works on my machine" syndrome combined with the very generic error message made identifying the root issue a lot more difficult than it should have been.

Therefore to reproduce this issue, you have to be on Windows (where no system-wide time zone database is available) and temporarily rename the folder $GOROOT/lib/time to something else.

[If you are only here because your calls to LoadLocation() return the same error, the solution to this problem is to import _ time/tzdata to embed a fallback copy of the database into the binary. See the time/tzdata package documentation for more details.]

package main

import (
	"fmt"
	"time"
)

func main() {
	_, err := time.LoadLocation("EST")
	fmt.Println("Error:", err)
}

What did you expect to see?

A clear error message that no time zone database is available.

What did you see instead?

Error: The system cannot find the path specified.

This error is very generic. Compare for example with os.Open(), which returns an error of the form open <filename>: The system cannot find the path specified.. Even better would be a specific error message for the case that none of the searched locations contained a time zone database.

Interestingly, if you rename the zip file inside $GOROOT/lib/time instead of the directory itself then you get a different nonsensical error Error: unknown time zone EST, because ENOENT is ignored here. I think separate cases are confused in this logic. In the case where the time zone database is a plain directory, ENOENT means that there is no file for this specific time zone, and ignoring the error is correct. However, ENOENT can also be returned from loadTzinfoFromZip if the zip file cannot be found, in which case I would expect the error to be returned further. The same problem can be found here regarding the ZONEINFO environment variable.

[rlanhellas]

Hi @GreenLightning, I agree with you, the error in windows is difficult to understand without knowledge of how Goland works with Timezone, the problem here is that Windows return different errors in different situations:

1. When the directory doesn't exist
   syscall.ENOTDIR

2. When the file doesn't exist
   syscall.ENOENT

But in Unix systems the return is always syscall.ENOENT. So, this line below, in loadLocation method, is just prepared for Unix systems:

if firstErr == nil && err != syscall.ENOENT {
			firstErr = err
		}

I did a test in my MacOS BigSur and Windows 10, with the line below working as expected in both systems:

if firstErr == nil && err != syscall.ENOENT && err != syscall.ENOTDIR {
			firstErr = err
		}

@ianlancetaylor , what do you think? Can I open a Merge Request with this fix and improve this error message?

[ianlancetaylor]

If we make such a change it should probably be done in time/sys_windows.go. It seems reasonable to do it there.

[rlanhellas]

I can't understand how changing time/sys_windows.go seems more reasonable, because methods like open call directly syscall.Open and the operational system returns ENOTDIR (3), there isn't a transformation or something else in sys_windows.go.

[GreenLightning]

My issue actually goes further than that.

1. No clear attribution of the error to the time package

Again, "The system cannot find the path specified." is a super generic error in Windows. My suggestion would be to wrap errors that are returned to the caller, e.g. like load location EST: The system cannot find the path specified. or similar. Compare with how os.Open() returns open example.txt: The system cannot find the path specified..

2. Behavior when no database is available

As mentioned in the original description ENOENT can mean two different things: a) the time zone database source is invalid, b) the source is valid, but does not contain an entry for this particular time zone.

I think ignoring the error in case b) is uncontroversial. Ignoring the error in case a) makes sense in the usual case (searching multiple common locations until a valid source is found). However, when all possible locations fail with case a) then I think a different error message should be returned, explicitly stating that no time zone database could be found instead of unknown time zone EST, which does not make any sense, because EST is a well-known time zone (or the error message should be changed, e.g. failed to load time zone EST).

Another case is when using ZONEINFO, if you misspell the value of ZONEINFO it will be silently ignored. Is this the intended behavior?

[rlanhellas]

My issue actually goes further than that.

1. No clear attribution of the error to the time package

Again, "The system cannot find the path specified." is a super generic error in Windows. My suggestion would be to wrap errors that are returned to the caller, e.g. like load location EST: The system cannot find the path specified. or similar. Compare with how os.Open() returns open example.txt: The system cannot find the path specified..

2. Behavior when no database is available

As mentioned in the original description ENOENT can mean two different things: a) the time zone database source is invalid, b) the source is valid, but does not contain an entry for this particular time zone.

I think ignoring the error in case b) is uncontroversial. Ignoring the error in case a) makes sense in the usual case (searching multiple common locations until a valid source is found). However, when all possible locations fail with case a) then I think a different error message should be returned, explicitly stating that no time zone database could be found instead of unknown time zone EST, which does not make any sense, because EST is a well-known time zone (or the error message should be changed, e.g. failed to load time zone EST).

Another case is when using ZONEINFO, if you misspell the value of ZONEINFO it will be silently ignored. Is this the intended behavior?

All the discussion is about improving error messages to help the developer solve problems in the fastest way. I really don't think misspelling ZONEINFO env should return an error because it can be another env variable and not a misspelling, so it's correct, we can't consider an error.

About improving messages to tell exactly error and not just a The system cannot find the path ..., I think we can change zoneinfo.go and zoneinfo_read.go to improve these messages, but I'm waiting to @ianlancetaylor explain the reason to make this change in time/sys_windows.go instead.

[ianlancetaylor]

@rlanhellas In the time package the files sys_unix.go and sys_windows.go are intended to hide the differences between Unix and Windows. Checking for syscall.ENOTDIR is only required on Windows. Therefore, I think we should hide that difference by changing the open function in sys_windows.go to check for syscall.ENOTDIR (or actually it would likely be more clear to check for syscall.ERROR_PATH_NOT_FOUND) and in that case to return syscall.ENOENT.

[rlanhellas]

I see, because in this case, we just need to change in exactly one point and everything that is using syscall.ENOTDIR keeps working normally.

I will suggest a Merge Request with this changing, ok @ianlancetaylor ?

[ianlancetaylor]

Sure, thanks, though likely won't be submitted until after the 1.18 release.

[rlanhellas]

Hello @ianlancetaylor , I made git clone and created my branch following this guide: https://go.dev/doc/contribute#sending_a_change_github. But when I try to send the push I got permission denied:

11:33:25.498: [go-github] git -c credential.helper= -c core.quotepath=false -c log.showSignature=false commit -F /private/var/folders/cq/n7ybvyp94_bfdsb_vx6984sw0000gn/T/git-commit-msg-.txt --
[fix-filenotfound-windows-timezonedb 9fee179f11] time: return ENOENT instead of ERROR_PATH_NOT_FOUND in windows
 1 file changed, 3 insertions(+)
11:33:27.381: [go-github] git -c credential.helper= -c core.quotepath=false -c log.showSignature=false push --progress --porcelain origin refs/heads/fix-filenotfound-windows-timezonedb:refs/heads/fix-filenotfound-windows-timezonedb --set-upstream
ERROR: Permission to golang/go.git denied to rlanhellas.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.

Any tips?

[seankhliao]

@rlanhellas Changes via github are made by using forks and PRs, not pushing directly to the this (read only) repo. See github docs for more information.