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

Disable importing legacy single-phase init extensions within subinterpreters in --disable-gil build #117649

Closed
colesbury opened this issue Apr 8, 2024 · 2 comments

Comments

@colesbury
Copy link
Contributor

colesbury commented Apr 8, 2024

Feature or enhancement

When importing a single-phase init extension in a subinterpreter, Python will make a shallow copy of the module's dictionary, which can share (non-immortal) objects between interpreters.

This does not work properly in the --disable-gil build, and we should disable it by raising an ImportError, at least for now. We can investigate how to support this in the future.

There are currently some unit tests for this case. Those tests pass, but that's mostly because they are simple and small changes to things like the GC will cause them to crash.

The underlying problems are not directly related to the GIL, but rather because the GC and our mimalloc integration in the --disable-gil build assume that non-immortal objects are isolated by interpreter:

  • The GC assumes that all tracked objects reachable via tp_traverse are also reachable from the per-interpreter mimalloc heaps. Violating this assumption can cause flags to be set to an inconsistent state.
  • The mimalloc pool of abandoned segments is per-interpreter. If a non-immortal object outlives its creating interpreter, this can cause use-after-free problems.

Linked PRs

@ericsnowcurrently
Copy link
Member

Note that this only relates to legacy subinterpreters (not isolated, created by Py_NewInterpreter()). With isolated interpreters single-phase init modules already trigger ImportError.

Raising ImportError under --disable-gil seems like a reasonable short-term solution.

@colesbury colesbury changed the title Disable importing single-phase init extensions in subinterpreters in --disable-gil build Disable importing legacy single-phase init extensions within subinterpreters in --disable-gil build Apr 8, 2024
colesbury added a commit to colesbury/cpython that referenced this issue Apr 8, 2024
…readed build

The free-threaded build does not currently support the combination of
single-phase init modules and legacy, non-isolated subinterpreters. Note
that with isolated interpreters, single-phase init modules already
trigger `ImportError`.
colesbury added a commit to colesbury/cpython that referenced this issue Apr 8, 2024
…readed build

The free-threaded build does not currently support the combination of
single-phase init modules and legacy, non-isolated subinterpreters. Note
that with isolated interpreters, single-phase init modules already
trigger `ImportError`.
@encukou
Copy link
Member

encukou commented Apr 9, 2024

You might want use the current mechanism: require PyInterpreterConfig.check_multi_interp_extensions to be 1 for Py_NewInterpreterFromConfig, and set it to 1 for Py_NewInterpreter (i.e. _PyInterpreterConfig_LEGACY_INIT).
And perhaps disable _imp._override_multi_interp_extensions_check.

Note that there are two ways to implement the limitation:

  • allow the import only in the main interpreter
  • allow the first import, regardless of what interp it's made in

The first was easier to implement; we might want to switch to the second later. If/when we do, --disable-gil builds might want to follow along.

colesbury added a commit to colesbury/cpython that referenced this issue Apr 11, 2024
colesbury added a commit that referenced this issue Apr 11, 2024
… build (#117651)

The free-threaded build does not currently support the combination of
single-phase init modules and non-isolated subinterpreters. Ensure that
`check_multi_interp_extensions` is always `True` for subinterpreters in
the free-threaded build so that importing these modules raises an
`ImportError`.
colesbury added a commit to colesbury/cpython that referenced this issue Apr 11, 2024
…case

The test case is currently expected to fail in the free-threaded build.
However, it fails before it gets a chance to close the write end of
the pipe.
colesbury added a commit that referenced this issue Apr 11, 2024
…117780)

The test case is currently expected to fail in the free-threaded build.
However, it fails before it gets a chance to close the write end of
the pipe.
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
…readed build (python#117651)

The free-threaded build does not currently support the combination of
single-phase init modules and non-isolated subinterpreters. Ensure that
`check_multi_interp_extensions` is always `True` for subinterpreters in
the free-threaded build so that importing these modules raises an
`ImportError`.
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
…case (python#117780)

The test case is currently expected to fail in the free-threaded build.
However, it fails before it gets a chance to close the write end of
the pipe.
@github-project-automation github-project-automation bot moved this from Todo to Done in Subinterpreters Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

3 participants