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

Fix Netkan timezones again #3246

Merged
merged 1 commit into from
Dec 21, 2020
Merged

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Dec 21, 2020

This is a spiritual successor to #3157.

Problem

A Discord user and I saw the same issue described in #3157: netkan.exe repeatedly purging and re-downloading a cache entry that it thought it was stale. The file was uploaded less than an hour previously, and I'm in the western hemisphere.

Cause

Here updatedToken will be a JToken object representing the (correct) value of release_date in the .ckan file, which includes timezone info:

JToken updatedToken;
DateTime t;
if (json.TryGetValue(UpdatedPropertyName, out updatedToken)
&& DateTime.TryParse(updatedToken.ToString(), out t))
{
RemoteTimestamp = t.ToUniversalTime();
}

To convert it to a DateTime, we:

  1. Cast it to a string, which internally results in something like 12/21/2020 7:53:07 PM, lacking timezone info
  2. Parse that string into a DateTime, which also lacks timezone info
  3. Call ToUniversalTime(), which adds (subtracts?) the local timezone offset to the timestamp, which for me was moving it into the future, which meant that my local file was always too old to result in a cache hit

Changes

Now we cast directly from JToken to DateTime, which is allowed and preserves the timezone info:

This fixed the mistaken cache purging in my testing.

@HebaruSan HebaruSan added Bug Something is not working as intended Pull request Netkan Issues affecting the netkan data Network Issues affecting internet connections of CKAN labels Dec 21, 2020
@DasSkelett
Copy link
Member

The Problem with Time & Timezones - Computerphile (Tom Scott)

Looking good code-wise, I'll give it a quick test run.

@HebaruSan
Copy link
Member Author

And we're not even trying to do any of the complicated stuff in that video, we're just trying to load a UTC timestamp and keep it in UTC all the way through the program.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Found a bug of #3223 while testing this (will note over there), but this PR on itself works fine 🎉 😄
Until someone on the ISS, whose clocks run on Astronomical Time comes by, at least.

@HebaruSan HebaruSan merged commit ad66e3c into KSP-CKAN:master Dec 21, 2020
@HebaruSan HebaruSan deleted the fix/netkan-tz branch December 21, 2020 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Netkan Issues affecting the netkan data Network Issues affecting internet connections of CKAN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants