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

PEP 489 -- Multi-phase extension module initialization #68456

Closed
encukou opened this issue May 23, 2015 · 40 comments
Closed

PEP 489 -- Multi-phase extension module initialization #68456

encukou opened this issue May 23, 2015 · 40 comments
Assignees
Labels
type-feature A feature request or enhancement

Comments

@encukou
Copy link
Member

encukou commented May 23, 2015

BPO 24268
Nosy @brettcannon, @ronaldoussoren, @ncoghlan, @scoder, @larryhastings, @encukou, @ericsnowcurrently, @zware, @zooba, @Carreau
Files
  • pep0489.patches
  • pep0489.patch
  • pep0489-multiphase-extension-module-import-v2.patch: Ready to commit (refleak in test_importlib)
  • refleak-fix.patch
  • fix-dynload-init-name.patch
  • fix-pep489-submodule.patch
  • fix-short-names.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/ncoghlan'
    closed_at = <Date 2015-05-26.12:00:28.742>
    created_at = <Date 2015-05-23.07:53:42.895>
    labels = ['type-feature']
    title = 'PEP 489 -- Multi-phase extension module initialization'
    updated_at = <Date 2015-06-03.22:31:45.117>
    user = 'https://github.com/encukou'

    bugs.python.org fields:

    activity = <Date 2015-06-03.22:31:45.117>
    actor = 'petr.viktorin'
    assignee = 'ncoghlan'
    closed = True
    closed_date = <Date 2015-05-26.12:00:28.742>
    closer = 'ncoghlan'
    components = []
    creation = <Date 2015-05-23.07:53:42.895>
    creator = 'petr.viktorin'
    dependencies = []
    files = ['39470', '39471', '39474', '39475', '39477', '39493', '39546']
    hgrepos = []
    issue_num = 24268
    keywords = ['patch']
    message_count = 40.0
    messages = ['243893', '243894', '243899', '243910', '243911', '243913', '243916', '243917', '243918', '243919', '243920', '243921', '243927', '243928', '243938', '243941', '243942', '243943', '243947', '243948', '243949', '243950', '243951', '243953', '243954', '243955', '243956', '243957', '243958', '244008', '244029', '244034', '244038', '244100', '244378', '244382', '244383', '244404', '244405', '244790']
    nosy_count = 12.0
    nosy_names = ['brett.cannon', 'ronaldoussoren', 'ncoghlan', 'scoder', 'larry', 'petr.viktorin', 'BreamoreBoy', 'python-dev', 'eric.snow', 'zach.ware', 'steve.dower', 'mbussonn']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue24268'
    versions = ['Python 3.5']

    @encukou
    Copy link
    Member Author

    encukou commented May 23, 2015

    Here is the implementation for the recently accepted PEP-489.

    Tested on Linux.

    @encukou
    Copy link
    Member Author

    encukou commented May 23, 2015

    And here are all changes in a single patch.

    @ncoghlan
    Copy link
    Contributor

    I'll get this merged tonight so we make the beta1 deadline, but I expect the initial docs to be fairly rudimentary and requiring further updates.

    @ncoghlan ncoghlan self-assigned this May 23, 2015
    @ncoghlan
    Copy link
    Contributor

    Attached patch is the one I'm looking to commit, but -R 3:3 shows significant reference leaks in test_importlib, and possible problems in other tests as well.

    I'm about to revert it to see if there were any pre-existing refleak issues before applying this.

    @ncoghlan
    Copy link
    Contributor

    Initial implementation checked in at https://hg.python.org/cpython/rev/e729b946cc03

    Larry, FYI regarding the refleak in test_importlib I just committed: as described in the commit message, I'm pretty sure it's a real refleak in the current PEP-489 implementation, it's just a beast to track down because there are a lot of moving parts when it comes to module cleanup, especially for extension modules.

    Aside from that, the tests all pass, so I hope you're OK with my checking it in its current state for beta 1, with the aim of fixing the bug for beta 2 (I would have asked first, but time zones intervened - I'm about to head to bed, and I expect you'll have closed 3.5 to new features by the time I get up)

    @ncoghlan
    Copy link
    Contributor

    Regarding the extraneous whitespace changes in modsupport.h, those are courtesy of running "make patchcheck" as part of preparing the commit.

    @encukou
    Copy link
    Member Author

    encukou commented May 23, 2015

    Fix some refleaks

    • one in PyModule_FromDefAndSpec2 - this is my fault
    • one in PyType_FromSpecWithBases - I guess this is
      the first time PEP-384 built-in types are GC'd
    • one in the new PEP-489 tests

    There's still one more in the new testing module, _testmultiphase. I'm working on it.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 23, 2015

    New changeset 7f2e6f236202 by Nick Coghlan in branch 'default':
    Issue bpo-24268: Address some PEP-489 refleaks
    https://hg.python.org/cpython/rev/7f2e6f236202

    @ncoghlan
    Copy link
    Contributor

    Thanks, that reduces the refleaks shown for "./python -m test -R3:3 test_importlib" from 102 to 50 for me.

    However, I suspect there may still be a leak in the machinery, as I'm still seeing the total number of objects growing when importing the array module and then dropping the references to it:

    $ ./python -X showrefcount
    Python 3.5.0a4+ (default:e729b946cc03+, May 24 2015, 00:58:18) 
    [GCC 5.1.1 20150422 (Red Hat 5.1.1-1)] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import sys
    [54384 refs, 15894 blocks]
    >>> import array; del array; del sys.modules['array']
    [54654 refs, 15974 blocks]
    >>> import array; del array; del sys.modules['array']
    [54695 refs, 15986 blocks]
    >>> import array; del array; del sys.modules['array']
    [54736 refs, 15997 blocks]
    >>> import array; del array; del sys.modules['array']
    [54777 refs, 16008 blocks]
    >>> import array; del array; del sys.modules['array']
    [54818 refs, 16019 blocks]
    >>> import array; del array; del sys.modules['array']
    [54859 refs, 16030 blocks]

    @ncoghlan
    Copy link
    Contributor

    Once we sort these out, any that impact Python 3.4 (like the PyType_FromSpecAndBases one) should be backported to the maintenance branch.

    I wouldn't assume the one you found in PyType_FromSpecAndBases is the only one, though - I believe the tests for this PEP are managing to exercise parts of the module cleanup machinery that it's never been practical to properly test in the past.

    And now I really am heading to bed :)

    @encukou
    Copy link
    Member Author

    encukou commented May 23, 2015

    The array module is good if you *really* drop references:

    $ ./python -X showrefcount
    Python 3.5.0a4+ (default, May 23 2015, 16:44:38) 
    [GCC 4.9.2 20150212 (Red Hat 4.9.2-6)] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import sys, gc
    [54618 refs, 15960 blocks]
    >>> import array; del array; del sys.modules['array']; gc.collect()
    6
    [54851 refs, 15973 blocks]
    >>> import array; del array; del sys.modules['array']; gc.collect()
    6
    [54851 refs, 15973 blocks]
    >>> import array; del array; del sys.modules['array']; gc.collect()
    6
    [54851 refs, 15973 blocks]

    There is a cycle between a each built-in function and its module. Modules aren't optimized for being unloaded.

    @encukou
    Copy link
    Member Author

    encukou commented May 23, 2015

    FWIW, the remaining refleak occurs when unloading an extension module object. This is something that wasn't possible before PEP-489 -- extension modules were never deleted.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 23, 2015

    New changeset a811f5561c99 by Steve Dower in branch 'default':
    Issue bpo-24268: Fixes generation of init import name on Windows.
    https://hg.python.org/cpython/rev/a811f5561c99

    @zooba
    Copy link
    Member

    zooba commented May 23, 2015

    For the sake of getting things running again, I went ahead and fixed the format string in dynload_win.c. Feel free to change it again if that isn't what was intended.

    @encukou
    Copy link
    Member Author

    encukou commented May 23, 2015

    Thank you, Steve.
    A similar problem is on other platforms as well. This patch should fix it; could someone look at it?

    @zooba
    Copy link
    Member

    zooba commented May 23, 2015

    That patch looks good to me. Totally didn't think to look for copy-paste issues...

    @larryhastings
    Copy link
    Contributor

    If it makes sense, can you guys check it in soon, like in real-time here? I tag 3.5 beta 1 in about an hour, and since this is a "bug-fix" it's legitimate to go in.

    @encukou
    Copy link
    Member Author

    encukou commented May 23, 2015

    Steve, could you please merge it?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 23, 2015

    New changeset 7b5f5f8b26a6 by Steve Dower in branch 'default':
    Issue bpo-24268: Fix import naming when loading extension modules. Patch by Petr Viktorin.
    https://hg.python.org/cpython/rev/7b5f5f8b26a6

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented May 23, 2015

    error C2079: 'PyModuleDef_Type' uses undefined struct '_typeobject' c:\cpython\include\moduleobject.h is occurring on both 32 and 64 bit release builds on Windows 8.1 VS2015. I don't know what is causing it but figured I'd better flag it up pronto, and as you guys are here...

    @zooba
    Copy link
    Member

    zooba commented May 23, 2015

    I think it's missing PyAPI_DATA, as it's probably supposed to be an external reference than a declaration. If it builds fine now, I'll commit that, but whoever made the change should confirm that's what it is supposed to be.

    @zooba
    Copy link
    Member

    zooba commented May 23, 2015

    It also required updating PC/python3.def to put the new functions into the stable API.

    This is the reason we should auto-gen that file (+Zach), to make sure that anything people can #include under the limited ABI can actually be used.

    Just double checking all the build configurations and then I'll push.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 23, 2015

    New changeset af167a62e2a3 by Steve Dower in branch 'default':
    Issue bpo-24268: Adds PyModuleDef_Init and PyModuleDef_Type to python3.def (stable ABI)
    https://hg.python.org/cpython/rev/af167a62e2a3

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented May 23, 2015

    FTR I've now built everything successfully.

    @ncoghlan
    Copy link
    Contributor

    Thanks for getting the Windows side sorted out folks, and my apologies for
    the breakage. The overlap between the current import system maintainers and
    Windows developers is unfortunately the null set :(

    @zooba
    Copy link
    Member

    zooba commented May 24, 2015

    Since you're up, any chance you can help diagnose these test failures:

    Traceback (most recent call last):
      File "D:\buildarea\3.x.bolen-windows8\build\lib\test\test_importlib\extension\test_loader.py", line 93, in setUp
        assert self.spec
    AssertionError

    From http://buildbot.python.org/all/builders/AMD64%20Windows8%203.x/builds/925/steps/test/logs/stdio

    It happens for 22 tests in test.test_importlib.extension.test_loader.Frozen_MultiPhaseExtensionModuleTests

    @zooba
    Copy link
    Member

    zooba commented May 24, 2015

    Actually, that probably means we're not building a new extension module on Windows, right?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 24, 2015

    New changeset 42ec976f627e by Steve Dower in branch 'default':
    Issue bpo-24268: Adds PCBuild project to build _testmultiphase module.
    https://hg.python.org/cpython/rev/42ec976f627e

    @zooba
    Copy link
    Member

    zooba commented May 24, 2015

    Confirmed that when we build the module needed by the test, the test works fine :)

    Unfortunately, I think it missed the branch for b1, so people who install and run the test suite will see failures there. I'll make sure it gets into the 3.5 branch once that opens up.

    @Carreau
    Copy link
    Mannequin

    Carreau mannequin commented May 24, 2015

    Hi,

    Since the last few patches related to this, I seem to have an issue with [Python/importdl.c](https://github.com/python/cpython/blob/main/Python/importdl.c):get_encoded_name (I guess) sometime returning the name with a leading dot. This lead to PyInit_.modulename being searched which fails.

    My C-foo is extremely low, but changing [1] to lastdot+1
    seem to do the trick for me (hence above supposition).

    More especially in my case compiling Cython failed to import Cython: dynamic module does not define module export function (PyInit_.Scanning), from import Cython.Compiler.Scanning I suppose. While it seem to does that ok with the +1.

    I haven't found any related issues, and my read of PEP-489 made me think this was not meant to change, Though I doubt my fix is correct.

    Sorry if duplicate or already fixed, I searched as I could but did not found anything.

    Thanks.

    [1] https://hg.python.org/cpython/rev/e729b946cc03#l29.59

    @encukou
    Copy link
    Member Author

    encukou commented May 25, 2015

    Yes, you did find an error. Thanks for reporting it!

    Here is a fix with a test case.

    @ronaldoussoren
    Copy link
    Contributor

    See bpo-24285, I ran into the same issue as mention in msg244008. That issue has a patch.

    @Carreau
    Copy link
    Mannequin

    Carreau mannequin commented May 25, 2015

    Yes, you did find an error. Thanks for reporting it!
    Here is a fix with a test case.

    Thanks, I was unsure if there would have been side effect or other things to fix. I would have submitted a patch otherwise.

    Thanks.

    @ncoghlan
    Copy link
    Contributor

    I registered the fix for importing extension modules from packages against bpo-24285 in the commit and NEWS file rather than against this original implementation RFE.

    Commit references in http://bugs.python.org/issue24285#msg244098

    @ncoghlan ncoghlan added the type-feature A feature request or enhancement label May 26, 2015
    @scoder
    Copy link
    Contributor

    scoder commented May 29, 2015

    I'm seeing crashes with this assertion in Cython test modules:

    python: ./Python/importdl.c:75: get_encoded_name: Assertion `(((PyObject*)(encoded))->ob_refcnt) == 1' failed.

    The problem seems to be that the module name is only one character long ("a"), and single character byte strings are interned.

    @encukou
    Copy link
    Member Author

    encukou commented May 29, 2015

    Ah, indeed. I need to create a new bytes object here after all.
    Here is a fix, with a test.

    Thank you Stefan!

    @scoder
    Copy link
    Contributor

    scoder commented May 29, 2015

    Patch LGTM and it fixes the problem (tried it on my side). Please make sure it gets applied in time for beta 2.

    @encukou
    Copy link
    Member Author

    encukou commented May 29, 2015

    I've opened bpo-24328 for that regression.

    @encukou
    Copy link
    Member Author

    encukou commented May 29, 2015

    And, the remaining refleak is a known issue from 2012: PyType_FromSpec-created types with custom tp_dealloc leak references when instantiated.
    See bpo-16690
    There's more discussion is in bpo-15653

    Martin v. Löwis notes:

    So I'd propose that it is actually the leaf subtype which decrefs
    ob_type. The check whether you are the leaf type is then done by
    checking whether tp_dealloc is the one you are "in" right now.

    Is that really the way to go? I didn't find this info in the docs, should those be updated?

    @encukou
    Copy link
    Member Author

    encukou commented Jun 3, 2015

    I've posted a patch that fixes the remaining refleak in bpo-24373.

    @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
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants