time: time.LoadLocation() is abusing opening files

[amezghal]

There is a problem with time.Location()

whenever it's called it will read the timezone info from disk.

but in a system which is abusing this, this will lead to max_open_files issues and will slow down the whole thing.

at least there must be a warning in the documentation of this function so users knows it. otherwise it will be very hard to debug.

[andybons]

It’s unclear to me how LoadLocation is “abusing” opening files. In the documentation for LoadLocation (there is no time.Location() function) it states:

The time zone database needed by LoadLocation may not be present on all systems, especially non-Unix systems. LoadLocation looks in the directory or uncompressed zip file named by the ZONEINFO environment variable, if any, then looks in known installation locations on Unix systems, and finally looks in $GOROOT/lib/time/zoneinfo.zip.

What do you suggest we add to the above documentation to make it clearer that it’s opening files?

[amezghal]

Hi, thank you for your reply.

We have this issue in our production code,

what we did is to create a map[string]*time.Location, in which we store the time.Location for a given timezone.

without this and because we rely on this a lot, our system start to fail overtime, and we are not able to get timezone info, because it opens a lot of file handles.

Reading the code of this function in the go stdlib you can see this.

if *zoneinfo != "" {
		if zoneData, err := loadTzinfoFromDirOrZip(*zoneinfo, name); err == nil {
			if z, err := LoadLocationFromTZData(name, zoneData); err == nil {
				return z, nil
			}
		}
	}

and

// loadTzinfoFromDirOrZip returns the contents of the file with the given name
// in dir. dir can either be an uncompressed zip file, or a directory.
func loadTzinfoFromDirOrZip(dir, name string) ([]byte, error) {
	if len(dir) > 4 && dir[len(dir)-4:] == ".zip" {
		return loadTzinfoFromZip(dir, name)
	}
	if dir != "" {
		name = dir + "/" + name
	}
	return readFile(name)
}

then

// readFile reads and returns the content of the named file.
// It is a trivial implementation of ioutil.ReadFile, reimplemented
// here to avoid depending on io/ioutil or os.
// It returns an error if name exceeds maxFileSize bytes.
func readFile(name string) ([]byte, error) {
	f, err := open(name)
	if err != nil {
		return nil, err
	}
	defer closefd(f)
	var (
		buf [4096]byte
		ret []byte
		n   int
	)
	for {
		n, err = read(f, buf[:])
		if n > 0 {
			ret = append(ret, buf[:n]...)
		}
		if n == 0 || err != nil {
			break
		}
		if len(ret) > maxFileSize {
			return nil, fileSizeError(name)
		}
	}
	return ret, err
}

as you can see it is opening files (maybe not on all systems but it does)

[andybons]

Thanks. How would you improve the documentation to better indicate that it’s opening files? The snippet I pasted above explains that calling LoadLocation looks for a database on disk and therefore implies it is reading from it. Is that language not clear enough?

…

On Fri, Apr 13, 2018 at 11:11 AM amezghal abdelilah < ***@***.***> wrote: Hi, thank you for your reply. We have this issue in our production code, what we did is to create a map[string]*time.Location, in which we store the time.Location for a given timezone. without this and because we rely on this a lot, our system start to fail overtime, and we are not able to get timezone info, because it opens a lot of file handles. Reading the code of this function in the go stdlib you can see this. if *zoneinfo != "" { if zoneData, err := loadTzinfoFromDirOrZip(*zoneinfo, name); err == nil { if z, err := LoadLocationFromTZData(name, zoneData); err == nil { return z, nil } } } and // loadTzinfoFromDirOrZip returns the contents of the file with the given name // in dir. dir can either be an uncompressed zip file, or a directory. func loadTzinfoFromDirOrZip(dir, name string) ([]byte, error) { if len(dir) > 4 && dir[len(dir)-4:] == ".zip" { return loadTzinfoFromZip(dir, name) } if dir != "" { name = dir + "/" + name } return readFile(name) } then // readFile reads and returns the content of the named file. // It is a trivial implementation of ioutil.ReadFile, reimplemented // here to avoid depending on io/ioutil or os. // It returns an error if name exceeds maxFileSize bytes. func readFile(name string) ([]byte, error) { f, err := open(name) if err != nil { return nil, err } defer closefd(f) var ( buf [4096]byte ret []byte n int ) for { n, err = read(f, buf[:]) if n > 0 { ret = append(ret, buf[:n]...) } if n == 0 || err != nil { break } if len(ret) > maxFileSize { return nil, fileSizeError(name) } } return ret, err } as you can see it is opening files (maybe not on all systems but it does) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#24844 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWSlGzOQHuI8XMPmF18tRc4yxd5iRLZks5toMAWgaJpZM4TTis6> .

[amezghal]

Thanks,

I would suggest to clearly warn users, that the call to this function might cost a file handle.
and depending on the use case, this might cause an issue (opening too much files at the same time that might exceed the system limits, and cause a side effect).

and to add a best practice / example, like bootstrapping data in a map to avoid relying on this.

or use a helper function that wrap and call time.LoadLocation() only once for a given timezone during the lifetime of the process.

In short: warn users that this might lead to issues if abused.

[andybons]

The documentation is clear in its behavior (it says that it will look for a database on disk and read from it if available).

I don't think it's our responsibility to warn users of this kind of abuse. It would set a precedent to add the same language under any function that opens an fd, allocates memory, executes arbitrary external processes, etc. Covering all vectors of abuse in documentation then becomes a fractal effort.

One can use a hammer to hang up a picture, or they can use it to break a window and enter another's home. For the same reason that there are no warning labels saying "please don't use this hammer to perform burglary" we abstain from providing warnings of the same ilk.

[panamafrancis]

I've created a copy of time.LoadLocation which loads all locations during init().

https://github.com/panamafrancis/tizzy

I found a similar issue to @amezghal in production, it's easy for people to use time.LoadLocation() without realising what it entails.

[amezghal]

Especially for people who are rewriting their code from other languages. we never encountered issues like this and we don't excpect this behavior. And the impact is big.
But anyway

[zhexuany]

@andybons Any update on this issue?

[ALTree]

@zhexuany This issue is closed so there won't be any more updates.

If you are asking for a faster LoadLocation with some kind of cache, please open a new issue. Explain what the problem is (e.g. "I call LoadLocation 3000 times per second, and it's too slow / opens too many files").