[subinterpreter] _PyUnicode_EqualToASCIIId() issue with subinterpreters

@srittau

BPO	46006
Nosy	@srittau, @vstinner, @encukou, @methane, @markshannon, @ericsnowcurrently, @serhiy-storchaka, @ndjensen, @hroncok, @corona10, @pablogsal, @erlend-aasland, @diabonas
PRs	bpo-46006: Fix _PyUnicode_EqualToASCIIId() for interned strings #30123 bpo-46006: Move the interned strings and identifiers to _PyRuntimeState. #30131 bpo-46006: Revert "bpo-40521: Per-interpreter interned strings (GH-20085)" #30422 [3.10] bpo-46006: Revert "bpo-40521: Per-interpreter interned strings (GH-20085)" (GH-30422) #30425 [3.10] bpo-46006: Revert "bpo-40521: Per-interpreter interned strings (GH-20085)" (GH-30422) #30433
Files	reproducer.c cmp_interned_strings.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2022-01-08.12:07:24.640>
created_at = <Date 2021-12-07.17:12:26.455>
labels = ['expert-subinterpreters', 'expert-C-API', '3.10', '3.11']
title = '[subinterpreter] _PyUnicode_EqualToASCIIId() issue with subinterpreters'
updated_at = <Date 2022-01-08.12:07:24.640>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2022-01-08.12:07:24.640>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2022-01-08.12:07:24.640>
closer = 'vstinner'
components = ['C API', 'Subinterpreters']
creation = <Date 2021-12-07.17:12:26.455>
creator = 'vstinner'
dependencies = []
files = ['50482', '50485']
hgrepos = []
issue_num = 46006
keywords = ['patch', '3.10regression']
message_count = 29.0
messages = ['407950', '407951', '407952', '407953', '407956', '408002', '408012', '408069', '408085', '408125', '408201', '408202', '408605', '408610', '408611', '408614', '408620', '408639', '408640', '408666', '408667', '409242', '409767', '409787', '409788', '409797', '409818', '409855', '409861']
nosy_count = 14.0
nosy_names = ['srittau', 'vstinner', 'craigh', 'petr.viktorin', 'methane', 'Mark.Shannon', 'eric.snow', 'serhiy.storchaka', 'ndjensen', 'hroncok', 'corona10', 'pablogsal', 'erlendaasland', 'diabonas']
pr_nums = ['30123', '30131', '30422', '30425', '30433']
priority = None
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue46006'
versions = ['Python 3.10', 'Python 3.11']

_PyUnicode_EqualToASCIIId() seems to be incompatible with subinterpreter: it makes the assumption that if direct pointer comparison fails and the string is interned, the two strings are not equal.

--

super_init_without_args() of Objects/typeobject.c calls _PyUnicode_EqualToASCIIId(name, &PyId___class__) to test if the Unicode string 'name' is equal to "__class__".

int
_PyUnicode_EqualToASCIIId(PyObject *left, _Py_Identifier *right)
{
    right_uni = _PyUnicode_FromId(right);
    ...
    if (left == right_uni)
        return 1;
    if (PyUnicode_CHECK_INTERNED(left))
        return 0;
    ...
    return unicode_compare_eq(left, right_uni);
}

_PyUnicode_EqualToASCIIId() makes the assumption that left and right are not equal if left and _PyUnicode_FromId(right) pointers are not equal and left is an interned string.

In the reproducer, left object is abc.ABCMeta.__new__.__code__.co_freevars[0].

Depending on how the stdlib abc.py file was loaded (in the main interpreter and in the subinterpreter), __code__.co_freevars[0] may or may not be an interned string.

If __code__.co_freevars[0] is an interned string, _PyUnicode_EqualToASCIIId() fails in a subinterpreter if the direct pointer comparison fails (if left and right_uni pointers are not equal).

--

Reproducer from: ninia/jep#358 (comment)

Build Python 3.10 with "./configure --enable-shared --prefix /opt/py310" and install it.
Download attached reproducer.c.
Build the reproducer with:
gcc -o reproducer reproducer.c $(/opt/py310/bin/python3.10-config --embed --cflags --ldflags)
Remove all stdlib .pyc files:
find /opt/py310 -type d -name __pycache__|xargs rm -rf
Run the reproducer with:
LD_LIBRARY_PATH=/opt/py310/lib ./reproducer

Output:
---

Before creating sub interpreter
Traceback (most recent call last):
  File "/opt/py310/lib/python3.10/io.py", line 52, in <module>
  File "/opt/py310/lib/python3.10/abc.py", line 184, in <module>
  File "/opt/py310/lib/python3.10/abc.py", line 106, in __new__
RuntimeError: super(): __class__ cell not found
Fatal Python error: _PyThreadState_Delete: tstate 0x7f9f2001c710 is still current
Python runtime state: initialized

Current thread 0x00007f9f27c99640 (most recent call first):
<no Python frame>
Abandon (core dumped)
---

py-bt command in gdb:
---

(gdb) py-bt
Traceback (most recent call first):
  File "/opt/py310/lib/python3.10/abc.py", line 106, in __new__
    cls = super().__new__(mcls, name, bases, namespace, **kwargs)
  <built-in method __build_class__ of module object at remote 0x7fffea0b4cc0>
  File "/opt/py310/lib/python3.10/abc.py", line 184, in <module>
    class ABC(metaclass=ABCMeta):
  <built-in method exec of module object at remote 0x7fffea0b4cc0>
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "/opt/py310/lib/python3.10/io.py", line 52, in <module>
    import abc
  <built-in method exec of module object at remote 0x7fffea0b4cc0>
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  <built-in method __import__ of module object at remote 0x7fffea0b4cc0>

In Python 3.9, the code works because the _Py_IDENTIFIER() API shares Python Unicode objects between all interpreters.

_PyUnicode_FromId() was modified to be per-interpreter in bpo-39465 by:

New changeset ba3d67c by Victor Stinner in branch 'master':
bpo-39465: Fix _PyUnicode_FromId() for subinterpreters (GH-20058)
ba3d67c

Serhiy: Do you recall the idea of the PyUnicode_CHECK_INTERNED() optimization?

The PyUnicode_CHECK_INTERNED() test is as old as the _PyUnicode_EqualToASCIIId() function.

commit f5894dd
Author: Serhiy Storchaka <storchaka@gmail.com>
Date: Wed Nov 16 15:40:39 2016 +0200

Issue bpo-28701: Replace _PyUnicode_CompareWithId with _PyUnicode_EqualToASCIIId.

The latter function is more readable, faster and doesn't raise exceptions.

Based on patch by Xiang Zhang.

Depending on how the stdlib abc.py file was loaded (in the main interpreter and in the subinterpreter), __code__.co_freevars[0] may or may not be an interned string.

When the bug occurs, I see that the Python stdlib abc.py file is loaded twice: the main interpreter builds a code object, and then subinterpreter builds its own code object: same content, but different Python object (at different memory addresses so inequal pointers!).

I modified reproducer.c to add "Py_VerboseFlag = 1;" before the Py_Initialize() call. Truncated output:
---

...
# code object from /opt/py310/lib/python3.10/abc.py
...
# code object from /opt/py310/lib/python3.10/abc.py
Traceback (most recent call last):
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
...
RuntimeError: super(): __class__ cell not found
...

There are around 27 _PyUnicode_EqualToASCIIId() calls in the Python code base. I don't think that avoiding _PyUnicode_EqualToASCIIId() is a good solution :-)

Fixing _PyUnicode_EqualToASCIIId() to make it compatible with subinterpreters sound more reasonable: remove the PyUnicode_CHECK_INTERNED() test optimization.

Should _PyUnicode_EqualToASCIIId() support comparing two unicode from different interpreter??

Should _PyUnicode_EqualToASCIIId() support comparing two unicode from different interpreter?

Right now, there still many cases where objects are still shared between two interpreters:

None, True, False singletons
strings from code objects (according to what I saw when I reproduced the issue)
objects from static types: type name (str), subtypes (tuple), MRO (tuple), etc.
etc.

More details in the following issues:

bpo-40533: [subinterpreters] Don't share Python objects between interpreters
bpo-40512: [subinterpreters] Meta issue: per-interpreter GIL

I've created a PR for moving the interned strings and identifiers to _PyRuntimeState until we are ready to move them back to the interpreter.

If that seems okay, I'll work on a backport PR for 3.10.

are there any objections to my PR?

Personally, I'd rather we not revert the original change.

Even in 3.10? Your PR looks pretty heavy for a bugfix release, and won't apply cleanly to 3.10.

Moving the data to _PyRuntimeState would save me some effort with related work I'm doing right now.

I guess that's what makes the PR hard to review. What's the plan (or are there more conflicting plans), and what's the state in 3.10 vs. main?

IMO writing a complete rationale for running multiple interpreters in parallel which require a whole PEP. I didn't write such PEP yet since there are still non-trivial technical issues, especially the problem of static types: bpo-40601.

I don't have time right now to measure the performance overhead of PR 30123 on _PyUnicode_EqualToASCIIId() and on changing a type attribute (like overriding the __init__() method). I expect a minor overhead on micro-benchmark and no significant change on pyperformance macrobenchmarks. But again, right I don't have time to go through such analysis.

The priority for now is to unblock the Python 3.11 release and repair the Python 3.10 regression, so I'm fine with reverting my commit ea25180 which introduced the regression.

PR 30131 makes more changes than just reverting the commit, it changes the _PyRuntimeState structure and it also reverts the identifiers change, whereas so far, there is no known relationship between this issue and identifiers. IMO it's ok to leave identifiers unchanged.

+1 on just reverting in both branches. I can deal with my stuff separately.

IMO writing a complete rationale for running multiple interpreters in parallel which require a whole PEP.

FYI, I'm planning on having such a PEP published in the next few days.

New changeset 35d6540 by Victor Stinner in branch 'main':
bpo-46006: Revert "bpo-40521: Per-interpreter interned strings (GH-20085)" (GH-30422)
35d6540

New changeset 72c260c by Victor Stinner in branch '3.10':
[3.10] bpo-46006: Revert "bpo-40521: Per-interpreter interned strings (GH-20085)" (GH-30422) (GH-30425)
72c260c

_PyUnicode_EqualToASCIIId() and type update_slot() functions are fixed in 3.10 and main branches. The regression is now fixed.

But the revert reintroduces the issue on subinterpreters, so I created bpo-46283: "[subinterpreters] Unicode interned strings must not be shared between interpreters".

Navigation Menu

Repository navigation

[subinterpreter] _PyUnicode_EqualToASCIIId() issue with subinterpreters #90164

14 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

[subinterpreter] _PyUnicode_EqualToASCIIId() issue with subinterpreters #90164

Description

Activity

vstinner commented on Dec 7, 2021

vstinner commented on Dec 7, 2021

vstinner commented on Dec 7, 2021

vstinner commented on Dec 7, 2021

vstinner commented on Dec 7, 2021

methane commented on Dec 8, 2021

vstinner commented on Dec 8, 2021

14 remaining items

ericsnowcurrently commented on Dec 16, 2021

ericsnowcurrently commented on Dec 16, 2021

ericsnowcurrently commented on Dec 27, 2021

encukou commented on Jan 5, 2022

vstinner commented on Jan 5, 2022

ericsnowcurrently commented on Jan 5, 2022

ericsnowcurrently commented on Jan 5, 2022

vstinner commented on Jan 6, 2022

vstinner commented on Jan 6, 2022

vstinner commented on Jan 6, 2022

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions