classification
Title: [subinterpreters] _PyImport_FixupExtensionObject() regression in Python 3.9
Type: behavior Stage: resolved
Components: Subinterpreters Versions: Python 3.11, Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.snow Nosy List: corona10, eric.snow, erlendaasland, jack__d, lukasz.langa, miss-islington, petr.viktorin, shihai1991, trygveaa, vstinner
Priority: normal Keywords: 3.9regression, patch

Created on 2021-05-05 14:22 by trygveaa, last changed 2021-10-05 20:42 by lukasz.langa. This issue is now closed.

Files
File name Uploaded Description Edit
interpreter.c trygveaa, 2021-05-05 14:22 C program to reproduce the issue
subinterpreter_ssl_issue.py trygveaa, 2021-05-05 14:22 Python script run from interpreter.c
Pull Requests
URL Status Linked Edit
PR 27794 merged shihai1991, 2021-08-17 12:24
PR 28738 merged miss-islington, 2021-10-05 13:19
PR 28741 merged lukasz.langa, 2021-10-05 16:34
Messages (15)
msg393011 - (view) Author: Trygve Aaberge (trygveaa) Date: 2021-05-05 14:22
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 https://github.com/python/cpython/commit/82c83bd907409c287a5bd0d0f4598f2c0538f34d
The commit that fixes the issue in 3.10 is https://github.com/python/cpython/commit/7f1305ef9ea7234e1a5aacbea17490232e9b7dc2

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
```
msg393012 - (view) Author: Trygve Aaberge (trygveaa) Date: 2021-05-05 14:22
Here is the Python script that the C program to reproduce the issue runs.
msg393013 - (view) Author: Trygve Aaberge (trygveaa) Date: 2021-05-05 14:31
And here are the bug reports for two Python scripts that are affected by this issue:
https://github.com/wee-slack/wee-slack/issues/812
https://github.com/poljar/weechat-matrix/issues/248
msg394310 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-05-25 10:15
> The commit that fixes the issue in 3.10 is https://github.com/python/cpython/commit/7f1305ef9ea7234e1a5aacbea17490232e9b7dc2

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 https://github.com/python/cpython/commit/82c83bd907409c287a5bd0d0f4598f2c0538f34d

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.
msg394374 - (view) Author: Trygve Aaberge (trygveaa) Date: 2021-05-25 16:14
> 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.
msg398030 - (view) Author: Jack DeVries (jack__d) * Date: 2021-07-23 05:22
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?
msg398895 - (view) Author: Trygve Aaberge (trygveaa) Date: 2021-08-04 13:19
@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).
msg399747 - (view) Author: Petr Viktorin (petr.viktorin) * (Python committer) Date: 2021-08-17 13:02
> 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 https://github.com/python/cpython/commit/82c83bd907409c287a5bd0d0f4598f2c0538f34d#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)
msg399748 - (view) Author: Hai Shi (shihai1991) * (Python triager) Date: 2021-08-17 13:04
> 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
msg399753 - (view) Author: Hai Shi (shihai1991) * (Python triager) Date: 2021-08-17 14:38
> (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 :)
msg403239 - (view) Author: miss-islington (miss-islington) Date: 2021-10-05 13:19
New changeset b9bb74871b27d9226df2dd3fce9d42bda8b43c2b by Hai Shi in branch 'main':
bpo-44050: Extension modules can share state when they don't support sub-interpreters. (GH-27794)
https://github.com/python/cpython/commit/b9bb74871b27d9226df2dd3fce9d42bda8b43c2b
msg403243 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-10-05 15:19
Thanks for the fix. I don't understand well this code :-(
msg403250 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-10-05 16:35
New changeset d0d29655ffc43d426ad68542d8de8304f7f1346a 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)
https://github.com/python/cpython/commit/d0d29655ffc43d426ad68542d8de8304f7f1346a
msg403266 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-10-05 20:30
New changeset 52d9d3b75441ae6038fadead89eac5eecdd34501 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)
https://github.com/python/cpython/commit/52d9d3b75441ae6038fadead89eac5eecdd34501
msg403268 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-10-05 20:42
Thanks, Hai Shi! ✨ 🍰 ✨
History
Date User Action Args
2021-10-05 20:42:37lukasz.langasetstatus: open -> closed
resolution: fixed
messages: + msg403268

stage: patch review -> resolved
2021-10-05 20:30:42lukasz.langasetmessages: + msg403266
2021-10-05 16:35:08lukasz.langasetmessages: + msg403250
2021-10-05 16:34:27lukasz.langasetpull_requests: + pull_request27088
2021-10-05 15:19:59vstinnersetmessages: + msg403243
2021-10-05 13:19:41miss-islingtonsetpull_requests: + pull_request27086
2021-10-05 13:19:39miss-islingtonsetnosy: + miss-islington
messages: + msg403239
2021-08-17 14:38:40shihai1991setmessages: + msg399753
2021-08-17 13:04:19shihai1991setmessages: + msg399748
2021-08-17 13:02:57petr.viktorinsetnosy: + petr.viktorin
messages: + msg399747
2021-08-17 12:24:48shihai1991setkeywords: + patch
stage: patch review
pull_requests: + pull_request26263
2021-08-04 13:19:16trygveaasetmessages: + msg398895
2021-07-23 05:22:34jack__dsetnosy: + jack__d
messages: + msg398030
2021-05-25 16:14:13trygveaasetmessages: + msg394374
2021-05-25 10:15:29vstinnersetnosy: + corona10, shihai1991, erlendaasland, - christian.heimes
title: Exceptions in a subinterpreter are changed by another subinterpreter -> [subinterpreters] _PyImport_FixupExtensionObject() regression in Python 3.9
messages: + msg394310

versions: + Python 3.10, Python 3.11
2021-05-05 18:54:47gregory.p.smithsetassignee: eric.snow

nosy: + eric.snow
2021-05-05 14:51:04kjsetkeywords: + 3.9regression
nosy: + vstinner, christian.heimes, lukasz.langa
2021-05-05 14:31:26trygveaasetmessages: + msg393013
2021-05-05 14:22:52trygveaasetfiles: + subinterpreter_ssl_issue.py

messages: + msg393012
2021-05-05 14:22:11trygveaacreate