Skip to content

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

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.

Sign up for GitHub

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

Merged
abalkin merged 7 commits into from
Jul 25, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8f163d3
Forego fold detection for timestamps in the range [0, 86399] on Windows.
ammaraskar Jun 24, 2017
488bc8b
Add test to ensure there is no OSError for a timestamp of 0
ammaraskar Jun 24, 2017
008778f
Add news entry, fix style nits
May 16, 2018
636ec5d
Add fold skipping in pure python datetime
May 16, 2018
9202447
Make Lib/datetime.py use a platform check
ammaraskar Jul 6, 2018
7241000
Adjust blurb message
ammaraskar Jul 6, 2018
64ae51a
Short cut windows check
ammaraskar Jul 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add news entry, fix style nits
  • Loading branch information
Ammar Askar committed May 16, 2018
commit 008778faf8d44e6f46c262f5ad207c098aacda96
2 changes: 0 additions & 2 deletions Lib/test/datetimetester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4944,13 +4944,11 @@ def test_fromtimestamp_lord_howe(self):
self.assertEqual(t0.fold, 0)
self.assertEqual(t1.fold, 1)


def test_fromtimestamp_low_fold_detection(self):

@berkerpeksag berkerpeksag May 16, 2018

Copy link
Copy Markdown
Member

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?

@ammaraskar ammaraskar May 16, 2018

Copy link
Copy Markdown
Member Author

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.

# Ensure that fold detection doesn't cause an
# OSError for really low values, see bpo-29097
self.assertEqual(datetime.fromtimestamp(0).fold, 0)


@support.run_with_tz('EST+05EDT,M3.2.0,M11.1.0')
def test_timestamp(self):
dt0 = datetime(2014, 11, 2, 1, 30)
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Windows/2018-05-16-11-31-17.bpo-29097.9mqEuI.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix bug where ``datetime.fromtimestamp`` erronously throws an OSError on

@berkerpeksag berkerpeksag Jul 6, 2018

Copy link
Copy Markdown
Member

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`

@berkerpeksag berkerpeksag Jul 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OSError -> :exc:`OSError`

@ammaraskar ammaraskar Jul 6, 2018

Copy link
Copy Markdown
Member Author

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.

Windows for values between 0 and 86400

@berkerpeksag berkerpeksag Jul 6, 2018

Copy link
Copy Markdown
Member

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.".

19 changes: 10 additions & 9 deletions Modules/_datetimemodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4627,15 +4627,16 @@ datetime_from_timet_and_us(PyObject *cls, TM_FUNC f, time_t timet, int us,
/* local timezone requires to compute fold */
if (tzinfo == Py_None && f == _PyTime_localtime
/* On Windows, passing a negative value to local results

@berkerpeksag berkerpeksag May 16, 2018

Copy link
Copy Markdown
Member

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
 * /

* in an OSError because localtime_s on Windows does
* not support negative timestamps. Unfortunately this
* means that fold detection for time values between
* 0 and max_fold_seconds will result in an identical
* error since we subtract max_fold_seconds 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. */
* in an OSError because localtime_s on Windows does
* not support negative timestamps. Unfortunately this
* means that fold detection for time values between
* 0 and max_fold_seconds will result in an identical
* error since we subtract max_fold_seconds 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.
*/
#ifdef MS_WINDOWS
&& (timet - max_fold_seconds > 0)
#endif
Expand Down