[subinterpreters] _PyImport_FixupExtensionObject() regression in Python 3.9

[trygveaa]

BPO	44050
Nosy	@vstinner, @encukou, @ambv, @ericsnowcurrently, @corona10, @miss-islington, @shihai1991, @erlend-aasland, @trygveaa, @jdevries3133
PRs	bpo-44050: Extension modules can share state when they don't support sub-interpreters. #27794 [3.10] bpo-44050: Extension modules can share state when they don't support sub-interpreters. (GH-27794) #28738 [3.9] bpo-44050: Extension modules can share state when they don't support sub-interpreters. (GH-27794) #28741
Files	interpreter.c: C program to reproduce the issue subinterpreter_ssl_issue.py: Python script run from interpreter.c

^{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 = 'https://github.com/ericsnowcurrently'
closed_at = <Date 2021-10-05.20:42:37.118>
created_at = <Date 2021-05-05.14:22:11.709>
labels = ['expert-subinterpreters', 'type-bug', '3.9', '3.10', '3.11']
title = '[subinterpreters] _PyImport_FixupExtensionObject() regression in Python 3.9'
updated_at = <Date 2021-10-05.20:42:37.117>
user = 'https://github.com/trygveaa'

bugs.python.org fields:

activity = <Date 2021-10-05.20:42:37.117>
actor = 'lukasz.langa'
assignee = 'eric.snow'
closed = True
closed_date = <Date 2021-10-05.20:42:37.118>
closer = 'lukasz.langa'
components = ['Subinterpreters']
creation = <Date 2021-05-05.14:22:11.709>
creator = 'trygveaa'
dependencies = []
files = ['50015', '50016']
hgrepos = []
issue_num = 44050
keywords = ['patch', '3.9regression']
message_count = 15.0
messages = ['393011', '393012', '393013', '394310', '394374', '398030', '398895', '399747', '399748', '399753', '403239', '403243', '403250', '403266', '403268']
nosy_count = 10.0
nosy_names = ['vstinner', 'petr.viktorin', 'lukasz.langa', 'eric.snow', 'corona10', 'miss-islington', 'shihai1991', 'erlendaasland', 'trygveaa', 'jack__d']
pr_nums = ['27794', '28738', '28741']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue44050'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

[trygveaa]

This issue is a regression in Python 3.9. It was recently fixed in main/3.10, but I'm opening this issue to request that it is fixed in 3.9 as well since it breaks certain Python scripts running in WeeChat.

I have a C application which is using the Python/C API and is running multiple python subinterpreters. One of those subinterpreters is running a script which is reading data from a non-blocking ssl socket. When there is no more data to read, trying to read throws ssl.SSLWantReadError which is handled by the script. However, if a script in another subinterpreter imports _ssl, the SSLWantReadError exceptions thrown in the first script now have a different class instance, so they are not catched anymore.

This is a regression in 3.9. It didn't happen in 3.8. The commit that introduced the issue is 82c83bd
The commit that fixes the issue in 3.10 is 7f1305e

I have attached a C program to reproduce the issue. It seems I can only attach one file per comment, so the python script that program runs will follow in the next commit. It connects to an ssl socket, so you need to have that running first. That can be started by running this:

openssl req -x509 -nodes -newkey rsa:4096 -keyout key.pem -out cert.pem -days 365 -subj '/'
openssl s_server -key key.pem -cert cert.pem -accept 1234

The script will output this when the issue is not present (so in 3.8 and main):

no data
no data

And this when the issue is present (in 3.9):

no dataunknown error: The operation did not complete (read) (_ssl.c:2627)
exception name: SSLWantReadError
SSLWantReadError id: 93893206532320
exception id: 93893207118800

[trygveaa]

Here is the Python script that the C program to reproduce the issue runs.

[trygveaa]

And here are the bug reports for two Python scripts that are affected by this issue:
wee-slack/wee-slack#812
poljar/weechat-matrix#248

[vstinner]

The commit that fixes the issue in 3.10 is 7f1305e

Right, the _ssl module was ported to multiphase initialization (PEP-489) in bpo-42333 (fixed in Python 3.10.0a3).

This is a regression in 3.9. It didn't happen in 3.8. The commit that introduced the issue is 82c83bd

In Python 3.9, some stdlib modules are still using the legacy API to initialize modules, and share states between interpreters. This is bad: no Python object (nor state) must be shared between two interpreters.

Well, there is an on-going work on subinterpreters:

• https://pyfound.blogspot.com/2021/05/the-2021-python-language-summit_16.html
• https://vstinner.github.io/isolate-subinterpreters.html

For example, many C extensions are being converted to the multiphase initialization API and get a "module state".

The _PyImport_FixupExtensionObject() change impacts extensions using the legacy API and so should not be used in subinterpreters.

Maybe the old behavior was better: if an extension uses the old API, share its state between all interpreters.

[trygveaa]

The _PyImport_FixupExtensionObject() change impacts extensions using the legacy API and so should not be used in subinterpreters.

I'm not using that directly in my code, but I guess it's used indirectly? If it shouldn't be used, what's the alternative in 3.9?

Maybe the old behavior was better: if an extension uses the old API, share its state between all interpreters.

Since this worked fine as far as I know before 3.9, and currently breaks existing applications, this is probably better yes.

[jdevries3133]

I just took a look at this, and I'm getting an output of "no data" (just one time) on 3.9.6. Has this been fixed?

[trygveaa]

@jack__d: If you're just getting "no data" once and nothing else, the test isn't working correctly. You should get "no data" twice when it works correctly and the issue is fixed, or "unknown error" and some other info if the issue is present. I'm not sure why it doesn't work for you though.

I tried it with Python 3.9.6 now, and I'm still seeing the issue, with the same output as in my original post (apart from a newline between "no data" and "unknown error", and a different line number and id numbers).

[encukou]

Maybe the old behavior was better: if an extension uses the old API, share its state between all interpreters.

Yes, I think the old behavior was better: if an extension uses the old API, share its state between all interpreters.

This is obviously bad, but I don't see how skipping part of initialization (as done in 82c83bd#diff-28cfc3e2868980a79d93d2ebdc8747dcb9231f3dd7f2caef96e74107d1ea3bf3L721-R719 ) is better.
(Note that the "def->m_size == -1" means that the module does not support sub-interpreters, because it has global state, per https://docs.python.org/3/c-api/module.html#c.PyModuleDef.m_size)

[shihai1991]

Maybe the old behavior was better: if an extension uses the old API, share its state between all interpreters.

+1. I create PR-27794, which use def->m_slots to identify the extension module created from PyModule_Create() or not. cc @petr

[shihai1991]

(Note that the "def->m_size == -1" means that the module does not support sub-interpreters, because it has global state.

Make Sense. It's more better and exact than my suggestion :)

[miss-islington]

New changeset b9bb748 by Hai Shi in branch 'main':
bpo-44050: Extension modules can share state when they don't support sub-interpreters. (GH-27794)
b9bb748

[vstinner]

Thanks for the fix. I don't understand well this code :-(

[ambv]

New changeset d0d2965 by Miss Islington (bot) in branch '3.10':
bpo-44050: Extension modules can share state when they don't support sub-interpreters. (GH-27794) (GH-28738)
d0d2965

[ambv]

New changeset 52d9d3b by Łukasz Langa in branch '3.9':
[3.9] bpo-44050: Extension modules can share state when they don't support sub-interpreters. (GH-27794) (GH-28741)
52d9d3b

[ambv]

Thanks, Hai Shi! ✨ 🍰 ✨