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

[subinterpreters] _PyImport_FixupExtensionObject() regression in Python 3.9 #88216

Closed
trygveaa mannequin opened this issue May 5, 2021 · 15 comments
Closed

[subinterpreters] _PyImport_FixupExtensionObject() regression in Python 3.9 #88216

trygveaa mannequin opened this issue May 5, 2021 · 15 comments
Assignees
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes topic-subinterpreters type-bug An unexpected behavior, bug, or error

Comments

@trygveaa
Copy link
Mannequin

trygveaa mannequin commented May 5, 2021

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
    Copy link
    Mannequin Author

    trygveaa mannequin commented May 5, 2021

    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 trygveaa mannequin added 3.9 only security fixes topic-subinterpreters type-bug An unexpected behavior, bug, or error labels May 5, 2021
    @trygveaa
    Copy link
    Mannequin Author

    trygveaa mannequin commented May 5, 2021

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

    @trygveaa
    Copy link
    Mannequin Author

    trygveaa mannequin commented May 5, 2021

    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
    Copy link
    Member

    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:

    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.

    @vstinner vstinner added 3.10 only security fixes 3.11 only security fixes labels May 25, 2021
    @vstinner vstinner changed the title Exceptions in a subinterpreter are changed by another subinterpreter [subinterpreters] _PyImport_FixupExtensionObject() regression in Python 3.9 May 25, 2021
    @vstinner vstinner added 3.10 only security fixes 3.11 only security fixes labels May 25, 2021
    @vstinner vstinner changed the title Exceptions in a subinterpreter are changed by another subinterpreter [subinterpreters] _PyImport_FixupExtensionObject() regression in Python 3.9 May 25, 2021
    @trygveaa
    Copy link
    Mannequin Author

    trygveaa mannequin commented May 25, 2021

    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
    Copy link
    Mannequin

    jdevries3133 mannequin commented Jul 23, 2021

    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
    Copy link
    Mannequin Author

    trygveaa mannequin commented Aug 4, 2021

    @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
    Copy link
    Member

    encukou commented Aug 17, 2021

    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
    Copy link
    Member

    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
    Copy link
    Member

    (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
    Copy link
    Contributor

    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
    Copy link
    Member

    vstinner commented Oct 5, 2021

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

    @ambv
    Copy link
    Contributor

    ambv commented Oct 5, 2021

    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
    Copy link
    Contributor

    ambv commented Oct 5, 2021

    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
    Copy link
    Contributor

    ambv commented Oct 5, 2021

    Thanks, Hai Shi! ✨ 🍰 ✨

    @ambv ambv closed this as completed Oct 5, 2021
    @ambv ambv closed this as completed Oct 5, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes topic-subinterpreters type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants