classification
Title: PEP 489 -- Multi-phase extension module initialization
Type: enhancement Stage: resolved
Components: Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: BreamoreBoy, brett.cannon, encukou, eric.snow, larry, mbussonn, ncoghlan, python-dev, ronaldoussoren, scoder, steve.dower, zach.ware
Priority: normal Keywords: patch

Created on 2015-05-23 07:53 by encukou, last changed 2015-06-03 22:31 by encukou. This issue is now closed.

Files
File name Uploaded Description Edit
pep0489.patches encukou, 2015-05-23 07:53
pep0489.patch encukou, 2015-05-23 07:58 review
pep0489-multiphase-extension-module-import-v2.patch ncoghlan, 2015-05-23 12:29 Ready to commit (refleak in test_importlib) review
refleak-fix.patch encukou, 2015-05-23 14:49
fix-dynload-init-name.patch encukou, 2015-05-23 19:35 review
fix-pep489-submodule.patch encukou, 2015-05-25 11:45 review
fix-short-names.patch encukou, 2015-05-29 14:46 review
Messages (40)
msg243893 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-23 07:53
Here is the implementation for the recently accepted PEP 489.

Tested on Linux.
msg243894 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-23 07:58
And here are all changes in a single patch.
msg243899 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-23 08:28
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.
msg243910 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-23 12:29
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.
msg243911 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-23 13:08
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)
msg243913 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-23 13:12
Regarding the extraneous whitespace changes in modsupport.h, those are courtesy of running "make patchcheck" as part of preparing the commit.
msg243916 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-23 14:49
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.
msg243917 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-23 15:03
New changeset 7f2e6f236202 by Nick Coghlan in branch 'default':
Issue #24268: Address some PEP 489 refleaks
https://hg.python.org/cpython/rev/7f2e6f236202
msg243918 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-23 15:07
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]
msg243919 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-23 15:12
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 :)
msg243920 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-23 15:20
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.
msg243921 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-23 15:24
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.
msg243927 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-23 16:00
New changeset a811f5561c99 by Steve Dower in branch 'default':
Issue #24268: Fixes generation of init import name on Windows.
https://hg.python.org/cpython/rev/a811f5561c99
msg243928 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-23 16:01
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.
msg243938 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-23 19:35
Thank you, Steve.
A similar problem is on other platforms as well. This patch should fix it; could someone look at it?
msg243941 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-23 20:17
That patch looks good to me. Totally didn't think to look for copy-paste issues...
msg243942 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2015-05-23 20:20
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.
msg243943 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-23 21:01
Steve, could you please merge it?
msg243947 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-23 21:14
New changeset 7b5f5f8b26a6 by Steve Dower in branch 'default':
Issue #24268: Fix import naming when loading extension modules. Patch by Petr Viktorin.
https://hg.python.org/cpython/rev/7b5f5f8b26a6
msg243948 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-05-23 21:14
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...
msg243949 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-23 21:27
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.
msg243950 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-23 21:37
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.
msg243951 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-23 21:44
New changeset af167a62e2a3 by Steve Dower in branch 'default':
Issue #24268: Adds PyModuleDef_Init and PyModuleDef_Type to python3.def (stable ABI)
https://hg.python.org/cpython/rev/af167a62e2a3
msg243953 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-05-23 22:31
FTR I've now built everything successfully.
msg243954 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-24 00:17
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 :(
msg243955 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-24 00:38
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
msg243956 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-24 00:39
Actually, that probably means we're not building a new extension module on Windows, right?
msg243957 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-24 00:52
New changeset 42ec976f627e by Steve Dower in branch 'default':
Issue #24268: Adds PCBuild project to build _testmultiphase module.
https://hg.python.org/cpython/rev/42ec976f627e
msg243958 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-05-24 01:01
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.
msg244008 - (view) Author: Matthias Bussonnier (mbussonn) * Date: 2015-05-24 23:36
Hi, 

Since the last few patches related to this, I seem to have an issue with `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
msg244029 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-25 11:45
Yes, you did find an error. Thanks for reporting it!

Here is a fix with a test case.
msg244034 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2015-05-25 13:37
See issue 24285, I ran into the same issue as mention in msg244008. That issue has a patch.
msg244038 - (view) Author: Matthias Bussonnier (mbussonn) * Date: 2015-05-25 14:45
> 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.
msg244100 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-26 12:00
I registered the fix for importing extension modules from packages against issue 24285 in the commit and NEWS file rather than against this original implementation RFE.

Commit references in http://bugs.python.org/issue24285#msg244098
msg244378 - (view) Author: Stefan Behnel (scoder) * Date: 2015-05-29 13:57
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.
msg244382 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-29 14:46
Ah, indeed. I need to create a new bytes object here after all.
Here is a fix, with a test.

Thank you Stefan!
msg244383 - (view) Author: Stefan Behnel (scoder) * Date: 2015-05-29 15:04
Patch LGTM and it fixes the problem (tried it on my side). Please make sure it gets applied in time for beta 2.
msg244404 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-29 19:51
I've opened issue24328 for that regression.
msg244405 - (view) Author: Petr Viktorin (encukou) * Date: 2015-05-29 19:57
And, the remaining refleak is a known issue from 2012: PyType_FromSpec-created types with custom tp_dealloc leak references when instantiated.
See issue 16690
There's more discussion is in issue 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?
msg244790 - (view) Author: Petr Viktorin (encukou) * Date: 2015-06-03 22:31
I've posted a patch that fixes the remaining refleak in issue24373.
History
Date User Action Args
2015-06-03 22:31:45encukousetmessages: + msg244790
2015-05-29 19:57:23encukousetmessages: + msg244405
2015-05-29 19:51:11encukousetmessages: + msg244404
2015-05-29 15:04:26scodersetmessages: + msg244383
2015-05-29 14:46:57encukousetfiles: + fix-short-names.patch

messages: + msg244382
2015-05-29 13:57:04scodersetnosy: + scoder
messages: + msg244378
2015-05-28 14:33:32brett.cannonsetnosy: + brett.cannon
2015-05-26 12:00:28ncoghlansetstatus: open -> closed
type: enhancement
messages: + msg244100

resolution: fixed
stage: resolved
2015-05-25 21:22:20berker.peksaglinkissue24285 superseder
2015-05-25 14:45:50mbussonnsetmessages: + msg244038
2015-05-25 13:37:53ronaldoussorensetnosy: + ronaldoussoren
messages: + msg244034
2015-05-25 11:45:41encukousetfiles: + fix-pep489-submodule.patch

messages: + msg244029
2015-05-24 23:36:15mbussonnsetnosy: + mbussonn
messages: + msg244008
2015-05-24 01:01:39steve.dowersetmessages: + msg243958
2015-05-24 00:52:32python-devsetmessages: + msg243957
2015-05-24 00:39:52steve.dowersetmessages: + msg243956
2015-05-24 00:38:25steve.dowersetmessages: + msg243955
2015-05-24 00:17:47ncoghlansetmessages: + msg243954
2015-05-23 22:31:07BreamoreBoysetmessages: + msg243953
2015-05-23 21:44:50python-devsetmessages: + msg243951
2015-05-23 21:37:00steve.dowersetnosy: + zach.ware
messages: + msg243950
2015-05-23 21:27:35steve.dowersetmessages: + msg243949
2015-05-23 21:14:48BreamoreBoysetnosy: + BreamoreBoy
messages: + msg243948
2015-05-23 21:14:19python-devsetmessages: + msg243947
2015-05-23 21:01:18encukousetmessages: + msg243943
2015-05-23 20:20:10larrysetmessages: + msg243942
2015-05-23 20:17:47steve.dowersetmessages: + msg243941
2015-05-23 19:35:27encukousetfiles: + fix-dynload-init-name.patch

messages: + msg243938
2015-05-23 16:01:14steve.dowersetnosy: + steve.dower
messages: + msg243928
2015-05-23 16:00:06python-devsetmessages: + msg243927
2015-05-23 15:24:27encukousetmessages: + msg243921
2015-05-23 15:20:10encukousetmessages: + msg243920
2015-05-23 15:12:16ncoghlansetmessages: + msg243919
2015-05-23 15:07:44ncoghlansetmessages: + msg243918
2015-05-23 15:03:59python-devsetnosy: + python-dev
messages: + msg243917
2015-05-23 14:49:40encukousetfiles: + refleak-fix.patch

messages: + msg243916
2015-05-23 13:12:10ncoghlansetmessages: + msg243913
2015-05-23 13:08:38ncoghlansetnosy: + larry
messages: + msg243911
2015-05-23 12:29:59ncoghlansetfiles: + pep0489-multiphase-extension-module-import-v2.patch

messages: + msg243910
2015-05-23 08:28:44ncoghlansetassignee: ncoghlan
messages: + msg243899
versions: + Python 3.5
2015-05-23 07:58:06encukousetfiles: + pep0489.patch
keywords: + patch
messages: + msg243894
2015-05-23 07:53:42encukoucreate