Skip to content
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

time.tzset does not reset _strptime's locale time cache #50727

Closed
mibanescu mannequin opened this issue Jul 13, 2009 · 15 comments
Closed

time.tzset does not reset _strptime's locale time cache #50727

mibanescu mannequin opened this issue Jul 13, 2009 · 15 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@mibanescu
Copy link
Mannequin

mibanescu mannequin commented Jul 13, 2009

BPO 6478
Nosy @abalkin, @bitdancer, @berkerpeksag, @serhiy-storchaka
Files
  • ttime.py: Reproducer for the problem.
  • _strptime.py.patch
  • issue6478.patch
  • issue6478_v2.patch
  • issue6478_v3.diff
  • strptime_cache_timezone.patch
  • 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 2015-12-12.08:25:22.817>
    created_at = <Date 2009-07-13.17:33:43.956>
    labels = ['type-bug', 'library']
    title = "time.tzset does not reset _strptime's locale time cache"
    updated_at = <Date 2015-12-12.08:25:22.816>
    user = 'https://bugs.python.org/mibanescu'

    bugs.python.org fields:

    activity = <Date 2015-12-12.08:25:22.816>
    actor = 'serhiy.storchaka'
    assignee = 'belopolsky'
    closed = True
    closed_date = <Date 2015-12-12.08:25:22.817>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2009-07-13.17:33:43.956>
    creator = 'mibanescu'
    dependencies = []
    files = ['14496', '17964', '27732', '27749', '29012', '41134']
    hgrepos = []
    issue_num = 6478
    keywords = ['patch']
    message_count = 15.0
    messages = ['90497', '110086', '110091', '173879', '173880', '173905', '173941', '174824', '174826', '181711', '209738', '229171', '255146', '255835', '256244']
    nosy_count = 6.0
    nosy_names = ['belopolsky', 'r.david.murray', 'mibanescu', 'python-dev', 'berker.peksag', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue6478'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5', 'Python 3.6']

    @mibanescu
    Copy link
    Mannequin Author

    mibanescu mannequin commented Jul 13, 2009

    If one changes from one timezone to another within the same python
    process, and if one tries to parse a time string that includes the
    timezone, the library malfunctions.

    See attached script for a reproducer.

    The problem is that, even though time.tzset() is called, the LocaleTime
    persisted in the TimeRE global is not reset. In my example, the EDT
    timezone name, compiled from the previous TZ variable, is not valid
    anymore in the 'Pacific/Fiji' timezone.

    To witness the behavior, run the attached script with no arguments. It
    will parse the time in the America/New_York timezone just fine.

    Then run it with an argument (like python ttime.py 1). It will first
    prime the _strptime cache in the Pacific/Fiji timezone, then attempt to
    parse the same time string in the America/New_York timezone.

    Finally, you can change the "if 0" to "if 1" for working around the problem.

    This has been verified in 2.4.4 and 2.6.1 (did not have a chance to
    verify it against python 2.6.2 yet).

    @mibanescu mibanescu mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Jul 13, 2009
    @mibanescu
    Copy link
    Mannequin Author

    mibanescu mannequin commented Jul 12, 2010

    Proposed patch attached.

    @abalkin abalkin self-assigned this Jul 12, 2010
    @abalkin
    Copy link
    Member

    abalkin commented Jul 12, 2010

    Thank you for the bug report and a patch.

    This does look like a bug and proposed patch seems to be the simplest way to fix it. It is unfortunate that the entire TimeRE cache needs to be recalculated when only 'Z' entry is invalidated by TZ change. Please consider separating lang and TZ change checks and not reinitialize TimeRE cache object if only TZ changes, just replace the 'Z' entry.

    The patch also needs unit tests. It should be straightforward to convert ttime.py into a test case. Please add a test case for both test_time and test_datetime. Note that in 3.2 datetime.strptime() will process '%Z' when '%z' is also present. For example,

    >>> datetime.strptime('Fri Jul 25 13:26:29 EDT -0500 2008', '%a %b %d %H:%M:%S %Z %z %Y')
    datetime.datetime(2008, 7, 25, 13, 26, 29, tzinfo=datetime.timezone(datetime.timedelta(-1, 68400), 'EDT'))

    Please make sure to restore environment variables in test cleanup.

    Also a nit on the implementation: cls is unused in _get_timezone(cls), so it would be more appropriate to make it staticmethod instead of classmethod. And tzset should probably be called inside _get_timezone.

    @berkerpeksag
    Copy link
    Member

    Here is the second patch with Alexander Belopolsky's comments addressed.

    @abalkin
    Copy link
    Member

    abalkin commented Oct 26, 2012

    The patch looks good, but I have a few comments on the test:

    1. Does it work on Windows? It seems to rely on Olson's TZ names.
    2. Please use @run_with_tz decorator.

    @berkerpeksag
    Copy link
    Member

    Thanks for the review.

    1. Does it work on Windows? It seems to rely on Olson's TZ names.

    Is time.tzset() function only available on Unix?

    See: http://docs.python.org/dev/library/time.html#time.tzset

    1. Please use @run_with_tz decorator.

    I will update my patch to use @run_with_tz decorator.

    @berkerpeksag
    Copy link
    Member

    1. Please use @run_with_tz decorator.

    Done. Patch attached.

    @berkerpeksag
    Copy link
    Member

    Alexander: Did you have a chance to review the test?

    @abalkin
    Copy link
    Member

    abalkin commented Nov 4, 2012

    issue6478_v2.patch looks good to me. There is a long line in _strptime.py which I will fix before committing.

    @berkerpeksag
    Copy link
    Member

    There is a long line in _strptime.py which I will fix before
    committing.

    Ops, fixed.

    Also, I've found a better place for the test in Lib/test/test_strptime.py
    and moved it to the test_strptime.CacheTests.

    When I applied the patch, test_bad_timezone test failed because of
    this code in Lib/_strptime.py:

        @staticmethod
        def _get_timezone():
            try:
                time.tzset()
            except AttributeError:
                pass
            # ...

    time.tzset() always resets time.tzname[1], so this part of the code
    in test_bad_timezone[2] didn't work.

        tzname = time.tzname[0]
        time.tzname = (tzname, tzname)

    I've fixed test_bad_timezone test by mocking the new
    _strptime._TimeRE_cache.locale_time._get_timezone method.

    [1] http://hg.python.org/cpython/file/default/Modules/timemodule.c#l827
    [2] http://hg.python.org/cpython/file/default/Lib/test/test_strptime.py#l320

    @serhiy-storchaka
    Copy link
    Member

    May be System Z Linux buildbot fails due to this bug (http://buildbot.python.org/all/builders/System%20Z%20Linux%203.x/builds/1154/steps/test/logs/stdio):

    ======================================================================
    ERROR: test_strptime (test.datetimetester.TestDateTime_Pure)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/home/dje/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/test/datetimetester.py", line 1879, in test_strptime
        dt = strptime(dtstr, "%z %Z")
      File "/home/dje/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/datetime.py", line 1593, in strptime
        return _strptime._strptime_datetime(cls, date_string, format)
      File "/home/dje/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/_strptime.py", line 500, in _strptime_datetime
        tt, fraction = _strptime(data_string, format)
      File "/home/dje/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/_strptime.py", line 337, in _strptime
        (data_string, format))
    ValueError: time data '-0500 EST' does not match format '%z %Z'

    @bitdancer
    Copy link
    Member

    Berker: the patch doesn't apply cleanly any more. Also, about the test_bad_timezone modification...what about the previous check that the tzname wasn't in UTC/GMT? Isn't that still needed? Or perhaps better yet, an additional @run_with_tz decorator? (Note that I don't really *understand* this code, so I may be totally off base here... :)

    @serhiy-storchaka
    Copy link
    Member

    I forgot about this issue and proposed similar patch for bpo-25168. It doesn't recalculate timezone values (a tuple of frozensets) on every _strptime call, but uses cheaper tests for time.tzname and time.daylight.

    issue6478_v3.diff has a bug, it doesn't clear _regex_cache after changing _TimeRE_cache. And I doubt that after fixing this the benefit of not recreating the entire _TimeRE_cache will be worth the complication of the code.

    Looks as this bug is a cause of random order depending tests failure (bpo-22067).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 3, 2015

    New changeset 2ae5c51c5dea by Serhiy Storchaka in branch '2.7':
    Issue bpo-6478: _strptime's regexp cache now is reset after changing timezone
    https://hg.python.org/cpython/rev/2ae5c51c5dea

    New changeset 4b0a4da1aa27 by Serhiy Storchaka in branch '3.4':
    Issue bpo-6478: _strptime's regexp cache now is reset after changing timezone
    https://hg.python.org/cpython/rev/4b0a4da1aa27

    New changeset 5fa855d20624 by Serhiy Storchaka in branch '3.5':
    Issue bpo-6478: _strptime's regexp cache now is reset after changing timezone
    https://hg.python.org/cpython/rev/5fa855d20624

    New changeset ea576db13827 by Serhiy Storchaka in branch 'default':
    Issue bpo-6478: _strptime's regexp cache now is reset after changing timezone
    https://hg.python.org/cpython/rev/ea576db13827

    @berkerpeksag
    Copy link
    Member

    Thanks Serhiy. Can we close this now?

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants