bpo-29097: Forego fold detection on windows for low timestamp values#2385

ammaraskar · 2017-06-24T23:02:34Z

See the bug tracker for more details on why I think this is a clean way to resolve this issue https://bugs.python.org/issue29097

https://bugs.python.org/issue29097

mention-bot · 2017-06-24T23:02:38Z

@ammaraskar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tim-one, @abalkin and @pitrou to be potential reviewers.

berkerpeksag

We also need a NEWS entry for this.

berkerpeksag · 2018-05-16T10:05:58Z

Lib/test/datetimetester.py

see issue #29097 -> see bpo-29097.

berkerpeksag · 2018-05-16T10:45:25Z

Lib/test/datetimetester.py

Should this test be marked as Windows-only?

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.

berkerpeksag · 2018-05-16T10:47:11Z

Modules/_datetimemodule.c

Style nit: Please use the existing comment style in the file:
/*firstline*secondline* /

bedevere-bot · 2018-05-16T10:47:38Z

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.

ammaraskar · 2018-05-16T20:11:17Z

#2530 revealed that the pure python version of fromtimestamp lacks a similar check, going to add it in.

ammaraskar · 2018-05-16T22:31:11Z

I have made the requested changes; please review again

bedevere-bot · 2018-05-16T22:31:13Z

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

abalkin · 2018-07-01T22:52:00Z

Lib/datetime.py

+ # than the max time fold. See comments in _datetimemodule's
+ # version of this method for more details.
+ try:
+ converter(-1)


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()?

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.

But this doesn't reflect the C implementation, which is only checking if it's being run on Windows.

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:
cpython/Lib/_bootlocale.py
Line 10 in 91106cd
ifsys.platform.startswith("win"):

berkerpeksag · 2018-07-06T08:44:09Z

Misc/NEWS.d/next/Windows/2018-05-16-11-31-17.bpo-29097.9mqEuI.rst

@@ -0,0 +1,2 @@
+Fix bug where ``datetime.fromtimestamp`` erronously throws an OSError on


It would be better if we can link to the fromtimestamp() documentation:
:meth:`datetime.fromtimestamp`

OSError -> :exc:`OSError`

Oh cool, didn't know we could use documentation syntax here.

berkerpeksag · 2018-07-06T08:45:18Z

Misc/NEWS.d/next/Windows/2018-05-16-11-31-17.bpo-29097.9mqEuI.rst

@@ -0,0 +1,2 @@
+Fix bug where ``datetime.fromtimestamp`` erronously throws an OSError on
+Windows for values between 0 and 86400


Please add "Patch by Ammar Askar.".

jleclanche · 2018-07-16T07:42:50Z

This could land in 3.7.1 or 3.7.2, yeah?

abalkin · 2018-07-24T23:10:28Z

Lib/datetime.py

+ # 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:


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?

Changed the order seeing as t < max_fold_seconds is a pretty rarely going to be true.

miss-islington · 2018-07-25T16:55:01Z

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!

…ythonGH-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>

bedevere-bot · 2018-07-25T16:55:12Z

GH-8466 is a backport of this pull request to the 3.7 branch.

miss-islington · 2018-07-25T16:57:55Z

Thanks @ammaraskar for the PR, and @abalkin for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington · 2018-07-25T16:57:58Z

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

…H-2385) (GH-8466) 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>

…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>

bedevere-bot · 2018-07-27T10:13:33Z

GH-8498 is a backport of this pull request to the 3.6 branch.

…alues (GH-2385) (GH-8498) 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>

the-knights-who-say-ni added the CLA signed label Jun 24, 2017

Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

jleclanche mentioned this pull request Dec 27, 2017
Windows Python 3.6 Datetime Bug HearthSim/python-hearthstone#24
Closed

brettcannon added the awaiting review label Feb 2, 2018

jleclanche mentioned this pull request Feb 23, 2018
Avoiding Python 3.6 bug HearthSim/python-hearthstone#25
Closed

berkerpeksag requested changes May 16, 2018
View reviewed changes

bedevere-bot removed the awaiting review label May 16, 2018

bedevere-bot added the awaiting changes label May 16, 2018

ammaraskar added 2 commits May 16, 2018 11:42

Add test to ensure there is no OSError for a timestamp of 0
488bc8b

ammaraskar force-pushed the fold_detection_windows branch from 11d1ddf to f60dbb1Compare May 16, 2018 18:46

Add news entry, fix style nits
008778f

ammaraskar force-pushed the fold_detection_windows branch from f60dbb1 to 008778fCompare May 16, 2018 18:47

ammaraskar force-pushed the fold_detection_windows branch 2 times, most recently from bae05d4 to 7c8847dCompare May 16, 2018 20:32

Add fold skipping in pure python datetime
636ec5d

ammaraskar force-pushed the fold_detection_windows branch from 7c8847d to 636ec5dCompare May 16, 2018 21:11

bedevere-bot added awaiting change review and removed awaiting changes labels May 16, 2018

terryjreedy requested a review from abalkin June 28, 2018 17:26

abalkin reviewed Jul 1, 2018
View reviewed changes

ammaraskar force-pushed the fold_detection_windows branch from 9be3a35 to 49a6937Compare July 6, 2018 01:02

Make Lib/datetime.py use a platform check
9202447

ammaraskar force-pushed the fold_detection_windows branch from 49a6937 to 9202447Compare July 6, 2018 01:16

berkerpeksag reviewed Jul 6, 2018
View reviewed changes

Adjust blurb message
7241000

berkerpeksag approved these changes Jul 6, 2018
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 6, 2018

abalkin reviewed Jul 24, 2018
View reviewed changes

Short cut windows check
64ae51a

abalkin self-assigned this Jul 25, 2018

abalkin added the needs backport to 3.7 label Jul 25, 2018

abalkin merged commit 96d1e69 into python:masterJul 25, 2018

bedevere-bot removed the awaiting merge label Jul 25, 2018

bedevere-bot removed the needs backport to 3.7 label Jul 25, 2018

abalkin added the needs backport to 3.6 label Jul 25, 2018

bedevere-bot removed the needs backport to 3.6 label Jul 27, 2018

dirkf mentioned this pull request Apr 5, 2023
[ARDMediathek] unhandled error in datetime.datetime.fromtimestamp() yt-dlp/yt-dlp#6706
Closed
11 tasks

		@@ -0,0 +1,2 @@
		Fix bug where ``datetime.fromtimestamp`` erronously throws an OSError on

		@@ -0,0 +1,2 @@
		Fix bug where ``datetime.fromtimestamp`` erronously throws an OSError on
		Windows for values between 0 and 86400

Uh oh!

bpo-29097: Forego fold detection on windows for low timestamp values#2385

bpo-29097: Forego fold detection on windows for low timestamp values #2385

Uh oh!

Conversation

ammaraskar commented Jun 24, 2017• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jun 24, 2017

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 16, 2018

Uh oh!

ammaraskar commented May 16, 2018

Uh oh!

ammaraskar commented May 16, 2018

Uh oh!

bedevere-bot commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleclanche commented Jul 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jul 25, 2018

Uh oh!

bedevere-bot commented Jul 25, 2018

Uh oh!

miss-islington commented Jul 25, 2018

Uh oh!

miss-islington commented Jul 25, 2018

Uh oh!

bedevere-bot commented Jul 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

ammaraskar commented Jun 24, 2017•
edited by bedevere-bot
Loading