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 DateMiddleWare cached timestamp return logic #1554

Merged
merged 2 commits into from Mar 20, 2018

Conversation

2 participants
@grundoon
Copy link
Member

grundoon commented Mar 17, 2018

createdAt <= date + accuracy will always evaluate to true, thus the cached timestamp value will always be returned, never recalculated.

Suggested date <= createdAt + accuracy will only return if accuracy threshold has not been exceeded, and otherwise fall through to create a new timestamp value.

Further discussion raised other potential issues:

A more human-readable way to write the logic might be (date..<(date + accuracy)).contains(createdAt)

(But is very slightly less efficient)

Here's another more readable form: date - createdAt < accuracy - however this suffers from the signed arithmetic problem where the system clock can be adjusted arbitrarily without warning.

(in fact the current way of tracking it is also susceptible to that - time() is not a monotonic clock)

The closed range form is the only one which will correctly toss the existing timestamp if the system clock is adjusted backwards by > 1 second (DST for example, or even general drift)

@grundoon grundoon changed the title fix cached timestamp return logic fix DateMiddleWare cached timestamp return logic Mar 17, 2018

@grundoon

This comment has been minimized.

Copy link
Member Author

grundoon commented Mar 17, 2018

Further commentary:

Use the (date..<(date + accuracy)).contains(createdAt) form instead, it's the most correct one
The efficiency difference should not be noticable.

(date..<(date + accuracy)).contains(createdAt) has it reversed too, no?
(createdAt..<(createdAt + accuracy)).contains(date)

Huh, you're right.

@grundoon

This comment has been minimized.

Copy link
Member Author

grundoon commented Mar 18, 2018

On further, further reflection (so sorry), the best solution really is the closed range form to catch system clock change, however there is no need to exclude the upper endpoint, since it simply defines equivalence:

(createdAt...(createdAt + accuracy)).contains(date)

Second commit added to reflect this.

@tanner0101 tanner0101 added the bug label Mar 20, 2018

@tanner0101 tanner0101 merged commit 16c4337 into vapor:nio Mar 20, 2018

2 checks passed

ci/circleci: linux Your tests passed on CircleCI!
Details
ci/circleci: linux-api-template Your tests passed on CircleCI!
Details

@tanner0101 tanner0101 added this to the 3.0.0-rc.2 milestone Mar 20, 2018

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Mar 20, 2018

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment