FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-44050: Extension modules can share state when they don't support sub-interpreters. by shihai1991 · Pull Request #27794 · python/cpython · GitHub

/ cpython Public

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

Merged
miss-islington merged 5 commits into
python:mainfrom
shihai1991:bpo_44050
Oct 5, 2021
Merged

bpo-44050: Extension modules can share state when they don't support sub-interpreters.#27794
miss-islington merged 5 commits into
python:mainfrom
shihai1991:bpo_44050

Conversation

shihai1991 commented Aug 17, 2021
edited by miss-islington
Loading

Copy link
Copy Markdown
Member

https://bugs.python.org/issue44050

Automerge-Triggered-By: GH:encukou

shihai1991 changed the title Extension modules created from PyModule_Create() can share state. bpo-44050: Extension modules created from PyModule_Create() can share state. Aug 17, 2021

encukou commented Aug 17, 2021

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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 commented Aug 17, 2021
edited
Loading

Copy link
Copy Markdown
Member

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 commented Aug 17, 2021
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

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

encukou commented Aug 17, 2021

Copy link
Copy Markdown
Member

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

encukou commented Aug 17, 2021

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

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 commented Aug 17, 2021

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

_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 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member Author

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.

shihai1991 changed the title bpo-44050: Extension modules created from PyModule_Create() can share state. bpo-44050: Extension modules doesn't support sub-interpreters can share state. Aug 17, 2021

encukou commented Aug 18, 2021

Copy link
Copy Markdown
Member

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.

shihai1991 closed this Aug 21, 2021

encukou commented Sep 7, 2021

Copy link
Copy Markdown
Member

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.

encukou removed the skip news label Sep 7, 2021

trygveaa commented Sep 7, 2021

Copy link
Copy Markdown

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?

Copy link
Copy Markdown
Member Author

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 commented Oct 5, 2021

Copy link
Copy Markdown
Member

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.

encukou changed the title bpo-44050: Extension modules can share state when it doesn't support sub-interpreters. bpo-44050: Extension modules can share state when they don't support sub-interpreters. Oct 5, 2021

Copy link
Copy Markdown
Contributor

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

miss-islington merged commit b9bb748 into python:main Oct 5, 2021

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 5, 2021
…sub-interpreters. (pythonGH-27794)

Automerge-Triggered-By: GH:encukou
(cherry picked from commit b9bb748)

Co-authored-by: Hai Shi <shihai1992@gmail.com>
miss-islington self-assigned this Oct 5, 2021

Copy link
Copy Markdown

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

bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 5, 2021

Copy link
Copy Markdown
Member Author

Thanks Petr, Victor for your review and merge.

ambv pushed a commit to ambv/cpython that referenced this pull request Oct 5, 2021
…pport sub-interpreters. (pythonGH-27794)

Automerge-Triggered-By: GH:encukou.
(cherry picked from commit b9bb748)

Co-authored-by: Hai Shi <shihai1992@gmail.com>

Copy link
Copy Markdown

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

ambv pushed a commit that referenced this pull request Oct 5, 2021
…sub-interpreters. (GH-27794) (GH-28738)

Automerge-Triggered-By: GH:encukou
(cherry picked from commit b9bb748)

Co-authored-by: Hai Shi <shihai1992@gmail.com>
ambv added a commit that referenced this pull request Oct 5, 2021
…pport sub-interpreters. (GH-27794) (GH-28741)

(cherry picked from commit b9bb748)

Co-authored-by: Hai Shi <shihai1992@gmail.com>

encukou commented Oct 12, 2021

Copy link
Copy Markdown
Member

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL