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.

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

Caching of parsed TimeZone information #728

Merged
merged 1 commit into from Jul 25, 2022
Merged

Caching of parsed TimeZone information #728

merged 1 commit into from Jul 25, 2022

Conversation

esheppa
Copy link
Collaborator

@esheppa esheppa commented Jul 5, 2022

This is a basic implementation of caching as described in #719

djc
djc reviewed Jul 5, 2022
View reviewed changes
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start! A bunch of thoughts...

src/offset/local/unix.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
djc
djc reviewed Jul 6, 2022
View reviewed changes
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking a lot better, some further suggestions/thoughts!

src/lib.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
src/offset/local/unix.rs Outdated Show resolved Hide resolved
djc
djc approved these changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@djc djc left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with one minor issue addressed!

src/offset/local/unix.rs Outdated Show resolved Hide resolved
@djc
Copy link
Contributor

djc commented Jul 25, 2022

@esheppa would you mind rebasing this? Probably also good to just squash the commits.

djc
djc reviewed Jul 25, 2022
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
@esheppa
use thread_local cache of parsed version of previous TzInfo file or T…
44e02a9 
…Z variable

minor fixes

improve ergonomics, cache mtime for 1s

1.32 fixes

avoid excess Cache::default() calls

add back cfg_attr
@esheppa esheppa merged commit 0903ab1 into chronotope:main Jul 25, 2022
@esheppa esheppa mentioned this pull request Jul 25, 2022
Closed
@djc djc mentioned this pull request Jul 26, 2022
Merged
@jhpratt
Copy link

jhpratt commented Jul 26, 2022

Is this PR not based on the assumption that the system's time zone will never change during the execution of a program? This is certainly not the case — the tzdb being updated by the system is the most obvious situation, but there are others that are far more subtle.

@djc
Copy link
Contributor

djc commented Jul 26, 2022 •
edited

We invalidate the cache within one second if the mtime of the tzdb file changes. Would be interested to hear more about the subtle scenarios you've considered for your solution in the time crate!

@jhpratt
Copy link

jhpratt commented Jul 26, 2022

Ah, invalidating the cache is good and should catch most cases.

The TZ environment variable could change. That is the core of the problem that led to the soundness issues, as I'm sure you are aware at this point.

When you say the "tzdb file", what are you referring to? The file containing the version? If so, it's possible for someone to write one of the other files directly. I don't blame you if you say "too bad" in that situation, but it's something to be mindful of.

@djc
Copy link
Contributor

djc commented Jul 26, 2022

The symlink metadata, see here: https://github.com/chronotope/chrono/pull/728/files#diff-568a8cdf0e0b7a4922b13d27771df35ac8e561ac9f25c5229263120edc171486R51.

We're explicitly leaving the TZ variable changing out of scope for now, because changing the environment seems so fraught with issues.

@M1cha
Copy link

M1cha commented Oct 4, 2022

In a proprietary project we're currently locked to chrono 0.4.19 because the recent addition of caching the timezone broke our code. I have to admit that the way we propagate the change to chrono is quite hacky and that we're relying on implementation details of both glibc and chrono.

So first of: yes, the TZ variable points to a symlink which systemd changes after updating the timezone. This is even the case on desktop arch linux.

So basically we're using dbus to listen for timezone changes that come from systemd-timesyncd.
Once that happened we need to make sure that localtime_r returns the updated value. The issue is that glibc caches the value, too. The difference is that they update it when the value of TZ has changed. So we can simply change the value, call tzset, change the value back, call tzset again.

That hack basically works for C and worked for chrono too because it always called localtime_r. Since that's not the case anymore, the change doesn't propagate to chrono anymore. So basically we need a way to propagate a timezone change to chrono.

@djc
Copy link
Contributor

djc commented Oct 5, 2022

@M1cha please file a new issue, or better yet, propose some code changes (likely around https://github.com/chronotope/chrono/blob/main/src/offset/local/unix.rs#L41) that would fix your issue as a PR. I'm surprised that it doesn't work, we designed the caching approach to be responsive to approaches such as yours.

@M1cha M1cha mentioned this pull request Oct 5, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@esheppa @djc @jhpratt @M1cha