-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 timezone handling for datetime to unixtime conversions #2213
Conversation
datetime objects are supported to set expire, these can have timezones. mktime was used to convert these to unixtime. mktime in Python however is not timezone aware, it expects the input to be UTC and redis-py did not convert the datetime timestamps to UTC before calling mktime. This can lead to: 1) Setting incorrect expire times because the input datetime object has a timezone but is passed to mktime without converting to UTC first. 2) When the datetime timestamp is within DST, mktime fails with "OverflowError: mktime argument out of range" because UTC doesn't have DST. This depends on libc versions.
@@ -1762,14 +1761,12 @@ def getex( | |||
if exat is not None: | |||
pieces.append("EXAT") | |||
if isinstance(exat, datetime.datetime): | |||
s = int(exat.microsecond / 1000000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed because the maxvalue for microsecond
is 999999 resulting in s
being 0 for any valid value.
Codecov Report
@@ Coverage Diff @@
## master #2213 +/- ##
==========================================
- Coverage 91.83% 91.82% -0.01%
==========================================
Files 108 108
Lines 27690 27683 -7
==========================================
- Hits 25429 25421 -8
- Misses 2261 2262 +1
Continue to review full report at Codecov.
|
pieces.append(exat) | ||
if pxat is not None: | ||
pieces.append("PXAT") | ||
if isinstance(pxat, datetime.datetime): | ||
ms = int(pxat.microsecond / 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed because timestamp()
returns a fractional result and multiplying with 1000 will therefore already consider ms.
@joekohlsdorf Sorry it took so long to respond. The PR looks good! I just merged master in to check if it works with the latest changes and we can approve & merge it |
Pull Request check-list
$ tox
pass with this change (including linting)?Description of change
datetime objects are supported to set expire, these can have timezones.
mktime was used to convert these to unixtime. mktime in Python however is not timezone aware, it expects the input to be UTC and redis-py did not convert the datetime timestamps to UTC before calling mktime.
This can lead to:
Since Python 3.3 datetime has a timestamp function which converts to UTC before calling mktime internally.
I changed all direct uses of mktime to use this instead.
How to reproduce the issues
DST:
No DST, conversion works but results in incorrect Unix timestamp due to non-timezone aware conversion: