bpo-29097: Forego fold detection on windows for low timestamp values by ammaraskar · Pull Request #2385 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need a NEWS entry for this.
| @@ -4361,6 +4361,13 @@ def test_fromtimestamp_lord_howe(self): |
|---|
| self.assertEqual(t1.fold, 1) |
| def test_fromtimestamp_low_fold_detection(self): |
| # Ensure that fold detection doesn't cause |
| # an OSError for really low values, see |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @@ -4361,6 +4361,13 @@ def test_fromtimestamp_lord_howe(self): |
|---|
| self.assertEqual(t1.fold, 1) |
| def test_fromtimestamp_low_fold_detection(self): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this test be marked as Windows-only?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily, while the bug is windows specific, if you look at the bpo thread you'll see there were no folds during this time. Therefore, this can just act as another unit test for fold detection on other platforms while being a regression test on windows.
| @@ -4286,7 +4286,21 @@ datetime_from_timet_and_us(PyObject *cls, TM_FUNC f, time_t timet, int us, |
|---|
| second = Py_MIN(59, tm.tm_sec); |
| /* local timezone requires to compute fold */ |
| if (tzinfo == Py_None && f == _PyTime_localtime) { |
| if (tzinfo == Py_None && f == _PyTime_localtime |
| /* On Windows, passing a negative value to local results |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: Please use the existing comment style in the file:
/* first line
- second line
- /
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
On Windows, passing a negative value to local results in an OSError because localtime_s on Windows does not support negative timestamps. Unfortunately this means that fold detection for timestamps between 0 and max_fold_seconds will result in this OSError since we subtract max_fold_seconds from the timestamp to detect a fold. However, since we know there haven't been any folds in the interval [0, max_fold_seconds) in any timezone, we can hackily just forego fold detection for this time range on Windows.
#2530 revealed that the pure python version of fromtimestamp lacks a similar check, going to add it in.
I have made the requested changes; please review again
Thanks for making the requested changes!
@berkerpeksag: please review the changes made to this pull request.
| # than the max time fold. See comments in _datetimemodule's |
|---|
| # version of this method for more details. |
| try: |
| converter(-1) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a way to detect windows or a windows-like system? Any such detection should be done in the module scope to avoid run-time penalty. Why not just check os.platform()?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the test suite uses sys.platform.startswith('win'), would that be acceptable? My reasoning for making it a generic detection was just in case any other platforms have the same problem, though that might be over-engineering it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this doesn't reflect the C implementation, which is only checking if it's being run on Windows.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, this is now a platform check. There is prior art in another stdlib module to suggest that the check from above is fine:
| if sys.platform.startswith("win"): |
|---|
| @@ -0,0 +1,2 @@ |
|---|
| Fix bug where ``datetime.fromtimestamp`` erronously throws an OSError on |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if we can link to the fromtimestamp() documentation:
:meth:datetime.fromtimestamp
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OSError -> :exc:`OSError`
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool, didn't know we could use documentation syntax here.
| @@ -0,0 +1,2 @@ |
|---|
| Fix bug where ``datetime.fromtimestamp`` erronously throws an OSError on |
| Windows for values between 0 and 86400 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add "Patch by Ammar Askar.".
This could land in 3.7.1 or 3.7.2, yeah?
| # thus we can't perform fold detection for values of time less |
|---|
| # than the max time fold. See comments in _datetimemodule's |
| # version of this method for more details. |
| if sys.platform.startswith("win") and t < max_fold_seconds: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we take the call to sys.platform.startswith() outside the function? Or at least change the order of and operands above so that it is not called every time?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the order seeing as t < max_fold_seconds is a pretty rarely going to be true.
Thanks @ammaraskar for the PR, and @abalkin for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
On Windows, passing a negative value to local results in an OSError because localtime_s on Windows does not support negative timestamps. Unfortunately this means that fold detection for timestamps between 0 and max_fold_seconds will result in this OSError since we subtract max_fold_seconds from the timestamp to detect a fold. However, since we know there haven't been any folds in the interval [0, max_fold_seconds) in any timezone, we can hackily just forego fold detection for this time range on Windows. (cherry picked from commit 96d1e69)
Co-authored-by: Ammar Askar ammar_askar@hotmail.com
Thanks @ammaraskar for the PR, and @abalkin for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖
Sorry, @ammaraskar and @abalkin, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.cherry_picker 96d1e69a12ed8ab80203277e1abdaf573457a964 3.6
abalkin pushed a commit that referenced this pull request
On Windows, passing a negative value to local results in an OSError because localtime_s on Windows does not support negative timestamps. Unfortunately this means that fold detection for timestamps between 0 and max_fold_seconds will result in this OSError since we subtract max_fold_seconds from the timestamp to detect a fold. However, since we know there haven't been any folds in the interval [0, max_fold_seconds) in any timezone, we can hackily just forego fold detection for this time range on Windows. (cherry picked from commit 96d1e69)
Co-authored-by: Ammar Askar ammar_askar@hotmail.com
ammaraskar added a commit to ammaraskar/cpython that referenced this pull request
…alues (pythonGH-2385)
On Windows, passing a negative value to local results in an OSError because localtime_s on Windows does not support negative timestamps. Unfortunately this means that fold detection for timestamps between 0 and max_fold_seconds will result in this OSError since we subtract max_fold_seconds from the timestamp to detect a fold. However, since we know there haven't been any folds in the interval [0, max_fold_seconds) in any timezone, we can hackily just forego fold detection for this time range on Windows.. (cherry picked from commit 96d1e69)
Co-authored-by: Ammar Askar ammar_askar@hotmail.com
abalkin pushed a commit that referenced this pull request
On Windows, passing a negative value to local results in an OSError because localtime_s on Windows does not support negative timestamps. Unfortunately this means that fold detection for timestamps between 0 and max_fold_seconds will result in this OSError since we subtract max_fold_seconds from the timestamp to detect a fold. However, since we know there haven't been any folds in the interval [0, max_fold_seconds) in any timezone, we can hackily just forego fold detection for this time range on Windows.. (cherry picked from commit 96d1e69)
Co-authored-by: Ammar Askar ammar_askar@hotmail.com
11 tasks