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

Add timezone support to datetime C API #54590

Closed
abalkin opened this issue Nov 10, 2010 · 14 comments
Closed

Add timezone support to datetime C API #54590

abalkin opened this issue Nov 10, 2010 · 14 comments
Assignees
Labels
extension-modules type-feature

Comments

@abalkin
Copy link
Member

@abalkin abalkin commented Nov 10, 2010

BPO 10381
Nosy @tim-one, @ncoghlan, @abalkin, @vstinner, @pganssle
PRs
  • #5032
  • #5317
  • #5318
  • #5814
  • #5819
  • #5929
  • 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 2018-07-05.16:52:42.095>
    created_at = <Date 2010-11-10.18:49:04.935>
    labels = ['extension-modules', 'type-feature']
    title = 'Add timezone support to datetime C API'
    updated_at = <Date 2018-07-05.16:52:42.094>
    user = 'https://github.com/abalkin'

    bugs.python.org fields:

    activity = <Date 2018-07-05.16:52:42.094>
    actor = 'vstinner'
    assignee = 'belopolsky'
    closed = True
    closed_date = <Date 2018-07-05.16:52:42.095>
    closer = 'vstinner'
    components = ['Extension Modules']
    creation = <Date 2010-11-10.18:49:04.935>
    creator = 'belopolsky'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 10381
    keywords = ['patch']
    message_count = 14.0
    messages = ['120928', '251873', '309214', '309234', '309264', '309267', '310639', '310662', '310676', '310688', '313020', '313022', '321105', '321117']
    nosy_count = 5.0
    nosy_names = ['tim.peters', 'ncoghlan', 'belopolsky', 'vstinner', 'p-ganssle']
    pr_nums = ['5032', '5317', '5318', '5814', '5819', '5929']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue10381'
    versions = ['Python 3.6']

    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Nov 10, 2010

    With timezone class added to datetime module, C API should be extended to at the minimum support efficient creation of timezone instances and access to the singleton UTC instance.

    I am not sure whether PyDateTime_TimeZone details should be exposed in datetime.h because presumably programmers should access it through the abstract tzinfo interface.

    @abalkin abalkin self-assigned this Nov 10, 2010
    @abalkin abalkin added extension-modules type-feature labels Nov 10, 2010
    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Sep 29, 2015

    Since bpo-24773 (PEP-495 implementation) will touch on CAPI, maybe it is time to implement this as well.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Dec 30, 2017

    Would it be possible to deprecate (or at least stop enhancing) the current datetime C API and add a new capsule based one instead?

    We're trying to get extension module authors to *stop* relying on C level globals, since it causes problems with:

    • memory usage analysis
    • interpreter reinitialisation
    • module reloading
    • subinterpreters

    Unfortunately, the current design of the datetime C API *requires* the use of C level global state.

    @pganssle
    Copy link
    Member

    @pganssle pganssle commented Dec 30, 2017

    @nick

    I'm certainly fine with re-configuring my PR to center around a new capsule module with deprecation of the old C API (though if you have any examples of what you have in mind that would be helpful to me). Certainly the "C global needs to be initiated" problem has bitten me in the past and I'm certain will continue to bite people in the future.

    That said, I think it would be really good if we could get a fast path for timezone creation and access to the UTC singleton into the Python 3.7 release. I think it's kind of a big disparity between the Python and C APIs that's existed for too long already.

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Dec 31, 2017

    On 31 December 2017 at 01:34, Paul Ganssle <report@bugs.python.org> wrote:

    That said, I think it would be really good if we could get a fast path for timezone creation and access to the UTC singleton into the Python 3.7 release. I think it's kind of a big disparity between the Python and C APIs that's existed for too long already.

    It turns out the API already *is* exported as a capsule:
    https://github.com/python/cpython/blob/master/Include/datetime.h#L149

    Our general guidance for capsule usage is just outdated, and the
    datetime API follows that dated advice:
    https://docs.python.org/3/extending/extending.html#providing-a-c-api-for-an-extension-module

    I'll file a separate issue for that docs problem later (the tracker is
    reporting 502 Proxy Error for me right now)

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Dec 31, 2017

    https://bugs.python.org/issue32459 covers defining a module-reloading-friendly way of using capsules.

    For this issue, I now think it makes sense to just ignore that problem, and add whatever you need to the existing API.

    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Jan 24, 2018

    New changeset 04af5b1 by Alexander Belopolsky (Paul Ganssle) in branch 'master':
    bpo-10381: Add timezone to datetime C API (bpo-5032)
    04af5b1

    @abalkin abalkin closed this Jan 24, 2018
    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 25, 2018

    Hunting reference leaks with "./python -m test -R 3:3 test_datetime" crashs since the commit 04af5b1.


    vstinner@apu$ ./python -m test -R 3:3 test_datetime
    Run tests sequentially
    0:00:00 load avg: 1.05 [1/1] test_datetime
    beginning 6 repetitions
    123456
    Fatal Python error: Segmentation fault

    Current thread 0x00007ff71fe33040 (most recent call first):
    File "/home/vstinner/prog/python/master/Lib/test/datetimetester.py", line 5464 in test_utc_capi
    (...)
    Segmentation fault (core dumped)
    ***

    Shorter example without -R, just run the same test twice:


    vstinner@apu$ ./python -m test -v test_datetime test_datetime -m test_utc_capi
    == CPython 3.7.0a4+ (heads/master:cab0b2b053, Jan 25 2018, 09:06:01) [GCC 7.2.1 20170915 (Red Hat 7.2.1-2)]
    == Linux-4.14.13-300.fc27.x86_64-x86_64-with-fedora-27-Twenty_Seven little-endian
    == cwd: /home/vstinner/prog/python/master/build/test_python_26308
    == CPU count: 8
    == encodings: locale=UTF-8, FS=utf-8
    Run tests sequentially
    0:00:00 load avg: 1.26 [1/2] test_datetime
    test_utc_capi (test.datetimetester.CapiTest_Pure) ... skipped 'Not relevant in pure Python'
    test_utc_capi (test.datetimetester.CapiTest_Fast) ... ok

    ----------------------------------------------------------------------
    Ran 2 tests in 0.001s

    OK (skipped=1)
    0:00:00 load avg: 1.26 [2/2] test_datetime
    test_utc_capi (test.datetimetester.CapiTest_Pure) ... skipped 'Not relevant in pure Python'
    test_utc_capi (test.datetimetester.CapiTest_Fast) ... Fatal Python error: Segmentation fault

    Current thread 0x00007f9836b31040 (most recent call first):
    File "/home/vstinner/prog/python/master/Lib/test/datetimetester.py", line 5464 in test_utc_capi
    (...)
    Segmentation fault (core dumped)
    ***

    @vstinner vstinner reopened this Jan 25, 2018
    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 25, 2018

    Ok, this time it should be fixed for real ;-)

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 25, 2018

    Oh, PR 5317 title mentionned this bpo number, but the merged change omits the bpo number and so wasn't mentioned on this issue.

    PR 5317 added:

    commit 58dc03c (upstream/master, master)
    Author: Paul Ganssle <pganssle@users.noreply.github.com>
    Date: Thu Jan 25 08:58:07 2018 -0500

    Cleanup dangling reference in get_timezone_utc_capi (bpo-5317)
    

    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Feb 27, 2018

    New changeset 5bd04f9 by Alexander Belopolsky (Paul Ganssle) in branch 'master':
    bpo-10381, bpo-32403: What's new entries for changes to datetime (gh-5814)
    5bd04f9

    @abalkin
    Copy link
    Member Author

    @abalkin abalkin commented Feb 27, 2018

    New changeset fff596f by Alexander Belopolsky (Miss Islington (bot)) in branch '3.7':
    bpo-10381, bpo-32403: What's new entries for changes to datetime (gh-5814) (gh-5929)
    fff596f

    @pganssle
    Copy link
    Member

    @pganssle pganssle commented Jul 5, 2018

    This can be closed now, I think.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jul 5, 2018

    I tested manually "./python -m test test_datetime test_datetime" in 3.6, 3.7 and master branches: I confirm that the test no longer crash. It has been fixed by the commit 58dc03c.

    Moreover, I don't recall a recent crash on Windows Refleak or Gentoo Refleak buildbots.

    Thanks again Paul Ganssle for this nice enhancement!

    @vstinner vstinner closed this Jul 5, 2018
    @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
    extension-modules type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants