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

Decide on caching behaviour for parsed tzinfo data #719

Closed
esheppa opened this issue Jun 25, 2022 · 5 comments
Closed

Decide on caching behaviour for parsed tzinfo data #719

esheppa opened this issue Jun 25, 2022 · 5 comments

Comments

@esheppa
Copy link
Collaborator

esheppa commented Jun 25, 2022

Currently we parse this lazily the first time that a user interacts with a Local timezone. This could be problematic in cases where the symlink for /etc/localtime is updated. See linked comment for further discussion

#683 (comment)

@djc
Copy link
Contributor

djc commented Jun 25, 2022

I don't think we really have natural API surface where this could be configured. Absent configuration API, I see 3 options:

  1. Always cache. Upside: only have to parse once, downside: timezone might become wrong if changed while app is running.
  2. Never cache: always in tune with system timezone, but getting the local timezone is relatively expensive.
  3. Cache with expiration: reparse current timezone at most once in, say, an hour, or a few minutes? Cost of parsing is amortized over many calls and app will be only be wrong for a little bit after timezone changes, some synchronization overhead and increased implementation complexity.

I wonder what libc does? Arguably we will attain maximum compatibility if we do the same thing as, say, glibc.

@esheppa
Copy link
Collaborator Author

esheppa commented Jun 25, 2022

From a brief look at glibc it seems to do the following:

  • each time that mktime is called, it calls tzset
  • tzset internally, whether TZ env variable or a tzfile is used, checks if it has been changed from the last call. If no change is detected it uses a cached version, otherwise it reparses the variable/file

We could potentially check the symlink metadata each time and only re-parse if it has been modified (in the case of using a file) which is I think quite similar to what glic does. chrono currently always reparses if using a TZ variable

@djc
Copy link
Contributor

djc commented Jun 27, 2022

Yeah, checking the symlink mtime (not sure it's always a symlink?) seems like a decent improvement. As a further optimization, I suppose we could only check the mtime once per minute or so?

We could take the Hasher::hash() in the case of a TZ variable, I suppose?

@djc
Copy link
Contributor

djc commented Jul 5, 2022

@esheppa would you be able to take a stab at this? IMO getting a 0.4.20 out with what we've got so far (especially the advisory fixes) is higher priority than triaging old pull issues/PRs.

@esheppa
Copy link
Collaborator Author

esheppa commented Jul 25, 2022

Solved via #728

@esheppa esheppa closed this as completed Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants