PEP 489 -- Multi-phase extension module initialization

[encukou]

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]

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

Tested on Linux.

[encukou]

And here are all changes in a single patch.

[ncoghlan]

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]

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]

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]

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

[encukou]

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]

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]

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]

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]

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]

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]

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]

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]

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

[zooba]

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

[larryhastings]

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]

Steve, could you please merge it?

[python-dev]

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]

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]

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]

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]

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]

FTR I've now built everything successfully.

[ncoghlan]

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]

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]

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

[python-dev]

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]

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]

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]

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

Here is a fix with a test case.

[ronaldoussoren]

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

[Carreau]

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]

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

[scoder]

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]

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

Thank you Stefan!

[scoder]

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

[encukou]

I've opened bpo-24328 for that regression.

[encukou]

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]

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