bpo-44050: Extension modules can share state when they don't support sub-interpreters.

[shihai1991]

https://bugs.python.org/issue44050

Automerge-Triggered-By: GH:encukou

[encukou]

I don't understand this PR. It allows Python objects to be shared between interpreters, doesn't it?

[shihai1991]

I don't understand this PR. It allows Python objects to be shared between interpreters, doesn't it?

Hi, petr. The details in this PR: 82c83bd
Only the main interp can update the extension and def.m_base.m_copy now. So I use the def->m_slots to identify the extension module created from PyModule_Create() or not. If extension module created from the PyModule_Create(), keep the old behavior. cc @vstinner

[encukou]

The relevant check is done by the def->m_size == -1. Extensions that use multi-phase init must set a non-negative m_size.

[shihai1991]

The relevant check is done by the def->m_size == -1. Extensions that use multi-phase init must set a non-negative m_size.

Hm. I agree with you. But the user of bpo-44050 got the error in v3.9.6. And I found some module created from PyModule_Create() use m_size. For example: https://github.com/python/cpython/blob/v3.9.6/Modules/_posixsubprocess.c#L986

[shihai1991]

This PR can be removed when all the extension modules convert to multi-phase init.

[encukou]

Why is it important to ask whether the module was created by PyModule_Create?

[encukou]

This PR can be removed when all the extension modules convert to multi-phase init.

That includes third-party modules, so: this PR can be removed when the API for single-phase init is removed.

[shihai1991]

Why is it important to ask whether the module was created by PyModule_Create?

Make sure only the single extension module shared between interpreters. The extension module from multi-phase init can be created seperately in subinterpreter, right?

[encukou]

All modules with non-negative m_size shouldn't be shared across interpreters, regardless of how they're created.

[shihai1991]

All modules with non-negative m_size shouldn't be shared across interpreters, regardless of how they're created.

Make sense. Looks your idea would be better. Thanks:) I will update it soon.

[vstinner]

_PyImport_FixupExtensionObject() copies the namespace from the first instance of an extension, to newly created instances of the extension. It's better than using exactly the same module object with the same dict dictionary object. In a perfect world, all extensions would use multi-phase init and so don't go through _PyImport_FixupExtensionObject().

[vstinner]

LGTM, but can you try to write an unit test?

Would it be possible to write an unit test for this? https://bugs.python.org/issue44050 is a regression. It would be better to not reintroduce it by mistake tomorrow.

Maybe using the ssl module: if if it's already imported, skip the test. Otherwise, spawn a subinterpreter to import it, and then import it in the main interpreter, and compare if you get the same objects or not.

See wee-slack/wee-slack#812 (comment) for a more complete example.

To make sure that the ssl module is not imported yet, you can run the test in a subprocess, eg. using assert_python_ok(), or subprocess.

[shihai1991]

LGTM, but can you try to write an unit test?

Would it be possible to write an unit test for this? https://bugs.python.org/issue44050 is a regression. It would be better to not reintroduce it by mistake tomorrow.

Maybe using the ssl module: if if it's already imported, skip the test. Otherwise, spawn a subinterpreter to import it, and then import it in the main interpreter, and compare if you get the same objects or not.

See wee-slack/wee-slack#812 (comment) for a more complete example.

To make sure that the ssl module is not imported yet, you can run the test in a subprocess, eg. using assert_python_ok(), or subprocess.

OK, victor. I will update it.

[encukou]

Maybe using the ssl module

That won't do; ssl uses multi-phase init (as should all other stdlib modules, one day). This would need to be a new module dedicated for the test.
You can look at Modules/_testmultiphase.c and Lib/test/test_capi.py for an example of adding a new module without adding a new shared library.

[encukou]

Looks good, although it looks like it'll only be backported to 3.10.1.
Could you add a NEWS entry, though? We should have those for bugfixes.

[trygveaa]

Looks good, although it looks like it'll only be backported to 3.10.1.

What do you mean? The regression is in 3.9, the issue is fixed in 3.10, and 3.10.1 doesn't exist yet. Did you mean 3.9.1?

[shihai1991]

Looks good, although it looks like it'll only be backported to 3.10.1.

What do you mean? The regression is in 3.9, the issue is fixed in 3.10, and 3.10.1 doesn't exist yet. Did you mean 3.9.1?

Maybe we cloud backport to 3.9 and 3.10?

[encukou]

What do you mean? The regression is in 3.9, the issue is fixed in 3.10, and 3.10.1 doesn't exist yet. Did you mean 3.9.1?

The "fix" in 3.10 is in the _ssl module only – an unrelated change that made the module avoid this bug.

Backporting to 3.10.1 & the next 3.9 sounds fine.

[miss-islington]

@shihai1991: Status check is done, and it's a success ✅ .

[miss-islington]

Thanks @shihai1991 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

[miss-islington]

Sorry, @shihai1991, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker b9bb74871b27d9226df2dd3fce9d42bda8b43c2b 3.9

[bedevere-bot]

GH-28738 is a backport of this pull request to the 3.10 branch.

[shihai1991]

Thanks Petr, Victor for your review and merge.

[bedevere-bot]

GH-28741 is a backport of this pull request to the 3.9 branch.

[encukou]

And thanks for your patience; this certainly waited for longer than it should have.