[Patch] Also stop using localtime() in timemodule

[EdSchouten]

BPO	28148
Nosy	@malemburg, @tim-one, @abalkin, @vstinner, @4kir4, @EdSchouten
PRs	[Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	patch-no-gmtime-localtime: Patch for using gmtime_r()/localtime_r() patch-pytime-localtime-gmtime

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/abalkin'
closed_at = <Date 2016-09-28.21:41:54.164>
created_at = <Date 2016-09-14.12:35:28.989>
labels = ['extension-modules', 'type-bug', '3.7']
title = '[Patch] Also stop using localtime() in timemodule'
updated_at = <Date 2017-03-31.16:36:25.789>
user = 'https://github.com/EdSchouten'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:25.789>
actor = 'dstufft'
assignee = 'belopolsky'
closed = True
closed_date = <Date 2016-09-28.21:41:54.164>
closer = 'belopolsky'
components = ['Extension Modules']
creation = <Date 2016-09-14.12:35:28.989>
creator = 'EdSchouten'
dependencies = []
files = ['44660', '44667']
hgrepos = []
issue_num = 28148
keywords = ['patch']
message_count = 14.0
messages = ['276421', '276471', '276472', '276474', '276475', '276479', '276480', '276484', '276487', '276488', '276489', '276490', '277663', '277665']
nosy_count = 7.0
nosy_names = ['lemburg', 'tim.peters', 'belopolsky', 'vstinner', 'akira', 'python-dev', 'EdSchouten']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue28148'
versions = ['Python 3.6', 'Python 3.7']

[EdSchouten]

In bpo-28067, we changed _datetimemodule to stop using localtime() and gmtime(), which is nice. I actually needed such a change for CloudABI (https://mail.python.org/pipermail/python-dev/2016-July/145708.html) which does not provide the thread-unsafe variants. Only localtime_r() and gmtime_r() are provided.

If we're starting to make use of these functions, let's complete this by also changing timemodule to use them. I've attached a patch.

Maybe it now makes sense to move the Windows wrappers to some header file, so that they can be shared between both modules. What would be the right location for this?

[abalkin]

Yes, I think this is a good idea. I was hesitant to make this change while bpo-22798 was open because I thought we may end up exposing changes to tzname caused by localtime and friends.

I also believe we can classify this as a bug-fix because side-effects of localtime are often undesirable. Nevertheless, I would like to hear from more people before accepting this.

[abalkin]

What would be the right location for [the Windows wrappers]?

I would say Include/pytime.h:

/**************************************************************************
Symbols and macros to supply platform-independent interfaces to time related
functions and constants
**************************************************************************/

[EdSchouten]

Hi Alexander,

I'm absolutely no expert when it comes to the Python codebase, so I've got a question. If we're going to movein this to Include/pytime.h, we should likely introduce full wrappers that have a name starting with _PyTime_, right?

This header seem to be part of the installation, so we should likely not declare any commonly used symbols there.

[abalkin]

we should likely introduce full wrappers that have a name starting with _PyTime_, right?

Yes, and I would like to give some thought to what the best API would be. The two choices are to emulate localtime_r on Windows or emulate localtime_s on POSIX. While localtime_r is probably a better known function, localtime_s has been standardized by C11 and may appear on POSIX platforms in the future.

Also, I think _PyTime_localtime_r/s should include the

 #ifdef EINVAL
         if (errno == 0)
             errno = EINVAL;
 #endif

code that is repeated everywhere in the current codebase.

[EdSchouten]

As a person who keeps a close eye on the Austin Group mailing lists (i.e., 'the POSIX working group'), my guess is that it's very unlikely that POSIX will ever add those *_s() extensions. Here's a discussion on Reddit that actually captures all of the arguments pretty well:

https://www.reddit.com/r/C_Programming/comments/3ivi77/eli5_why_does_glibc_still_not_support_the/

That said, any API will do. The localtime_r() function has the disadvantage that the return value is a bit odd: can it return any other tm structure than the one provided? localtime_s() is a bit weird in that its input argument is stored after the output argument. Both functions also unnecessarily pass the time_t by reference. Maybe we can just pick a prototype that's as Pythonesque as possible that also fixes these shortcomings. Any thoughts?

[abalkin]

Maybe we can just pick a prototype that's as Pythonesque as possible that also fixes these shortcomings. Any thoughts?

Yes, and just call it _PyTime_localtime without the ugly suffix.

[EdSchouten]

Does this patch look all right to you?

[abalkin]

Very nice. I'll give it a week or two for the others to chime in and commit if I don't hear any objections.

[abalkin]

I see that you picked localtime_s-like order of arguments. While I have no personal preference, I wonder why you prefer output to follow input. The usual UNIX/C convention is the opposite. Interfaces like sprintf, strcat, strftime and many other have output first. I think the logic is that this is the order in variable assignment OUTPUT = INPUT.