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

Add a Check for Extensions that Support Running in Subinterpreters #98627

Open
ericsnowcurrently opened this issue Oct 24, 2022 · 4 comments
Open
Assignees
Labels
3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) tests Tests in the Lib/test dir topic-subinterpreters type-feature A feature request or enhancement

Comments

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Oct 24, 2022

Any given extension module may or may not support being loaded in multiple interpreters. This primarily depends on whether or not the extension use any process-global state. When imported in multiple interpreters at the same time, a module that does not isolate its state may lead to problems.

Global state (which must be isolated) includes:

  • Python objects the extension uses (including internally)
  • other data it uses directly
  • indirect state, via linked libraries

If an extension properly implements multi-phase init (PEP 489) then it probably does support use in multiple interpreters (but not necessarily under per-interpreter GIL). In fact, PEP 489 proscribes this.

It would be useful to add a check to the ExtensionFileLoader to this effect.

Proposal

  • if an extension is imported in a sub-interpreter and it does not implement multi-phase init then we should raise ImportError
  • this should be configurable on a per-interpreter basis
  • the default is debatable
  • we may want to allow multi-phase init extension to opt in to multiple interpreter support
  • (there are some additional minor considerations if we factor in a per-interpreter GIL)

Configuration

Given a _PyInterpreterConfig (see gh-98608), we would add something like _PyInterpreterConfig.check_multi_interp_extensions to indicate that the interpreter created via _PyInterpreterNewFromConfig() should do the check or not.

For backward compatibility, we'd use the following defaults:

  • "false" for the main interpreter
  • (maybe) "false" for the legacy Py_NewInterpreter()
  • "true" for "isolated" interpreters (see the _xxsubinterpreters module, PEPs 554, 684)

For Py_NewInterpreter() it may make sense to default to "true", to avoid causing folks headaches (see pyca/cryptography#2299). However, we'll probably want to stick with "false" under the assumption that most extensions work well enough in multiple interpreters to allow us to preserve compatibility.

A Module Def Slot

Per PEP 684 discussions, there is some uncertainty about how confident extension authors may be about how isolated their extensions really are. If this is a genuine concern then we should consider also adding a PEP 489 module def slot, e.g. Py_mod_subinterpreters, to allow extension authors to opt in to supporting multiple interpreters. It would probably also make sense to offer a slot to opt out of multiple interpreter support, e.g. Py_mod_no_subinterpreters, for when support is more wide-spread.

Again, this only makes sense if we aren't confident that multi-phase init support is an adequate indicator.

Per-Interpreter GIL (PEP 684)

There are additional static variables, e.g. temporary buffers, that factor in to extension "isolation" if you drop the thread safety provided by the GIL. This is real concern relative to PEP 684, where we would make the GIL per-interpreter. The difference between isolation is relatively small, but enough to warrant consideration.

Most notably, a per-interpreter GIL would strengthen the value of a dedicated module def slot.

@ericsnowcurrently ericsnowcurrently added type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-subinterpreters 3.12 bugs and security fixes labels Oct 24, 2022
@ericsnowcurrently
Copy link
Member Author

CC @encukou

@ericsnowcurrently
Copy link
Member Author

ericsnowcurrently commented Oct 25, 2022

@ericsnowcurrently ericsnowcurrently self-assigned this Oct 25, 2022
ericsnowcurrently added a commit that referenced this issue Oct 27, 2022
This helps simplify some changes in follow-up PRs.  It also matches what we're doing in PyModule_ExecDef().
@encukou
Copy link
Member

encukou commented Oct 27, 2022

Without per-interpreter GIL, I don't think the slot (and check) is worth the effort.
With per-interpreter GIL, the slot name should probably be more tied to that. If it supports per-interpreter GIL, it better support subinterpreters as well -- and the in-between state (multiple interpreters but not multi-gil) is not worth it for extension authors to verify and signing off on (using the slot).
Maybe Py_mod_isolated?

Another point: some people did things correctly, despite lack of good guidance, and it's not nice to ask them to reassert that their extension supports multiple interpreters, when they already did that with multi-phase initialization.

(maybe) "false" for the legacy Py_NewInterpreter()

Yes, this needs to be false for backwards compatibility. Perhaps with an -X flag to control it, or a disallow counterpart for the PEP 684 importlib.util.allow_all_extensions, to more easily test things like mod_wsgi workloads.

gvanrossum pushed a commit to gvanrossum/cpython that referenced this issue Oct 28, 2022
…98734)

This helps simplify some changes in follow-up PRs.  It also matches what we're doing in PyModule_ExecDef().
@ericsnowcurrently
Copy link
Member Author

Without per-interpreter GIL, I don't think the slot (and check) is worth the effort.

Fine with me. I guess I misunderstood something you said elsewhere.

With per-interpreter GIL, the slot name should probably be more tied to that.

Agreed.

Maybe Py_mod_isolated?

I'd consider "isolated" to match multiple interpreter support. Something like "Py_mod_thread_safe" or seems more clear for a per-interpreter GIL.

Another point: some people did things correctly, despite lack of good guidance, and it's not nice to ask them to reassert that their extension supports multiple interpreters, when they already did that with multi-phase initialization.

Agreed.

Yes, this needs to be false for backwards compatibility.

Yeah, I'm not sure why I put "(maybe)". I was probably thinking of saving folks like cryptography from headache. However, that would be too disruptive for legacy users.

Perhaps with an -X flag to control it, or a disallow counterpart for the PEP 684 importlib.util.allow_all_extensions, to more easily test things like mod_wsgi workloads.

Good idea. I'll look into that.

ericsnowcurrently added a commit that referenced this issue Nov 8, 2022
This makes it more clear that a given test is definitely testing against a single-phase init (legacy) extension module.  The new module is a companion to _testmultiphase.

#98627
ericsnowcurrently added a commit that referenced this issue Feb 16, 2023
…ompatibility (gh-99040)

Enforcing (optionally) the restriction set by PEP 489 makes sense. Furthermore, this sets the stage for a potential restriction related to a per-interpreter GIL.

This change includes the following:

* add tests for extension module subinterpreter compatibility
* add _PyInterpreterConfig.check_multi_interp_extensions
* add Py_RTFLAGS_MULTI_INTERP_EXTENSIONS
* add _PyImport_CheckSubinterpIncompatibleExtensionAllowed()
* fail iff the module does not implement multi-phase init and the current interpreter is configured to check

#98627
@erlend-aasland erlend-aasland added the tests Tests in the Lib/test dir label Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) tests Tests in the Lib/test dir topic-subinterpreters type-feature A feature request or enhancement
Projects
Status: Todo
Development

No branches or pull requests

3 participants