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

Jump to bottom

Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} #77806

Closed
william-gr mannequin opened this issue May 23, 2018 · 13 comments
Closed

Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} #77806

william-gr mannequin opened this issue May 23, 2018 · 13 comments
Labels
3.7 (EOL) end of life extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@william-gr
Copy link
Mannequin

william-gr mannequin commented May 23, 2018

BPO 33625
Nosy @vstinner, @tiran, @ned-deily, @serhiy-storchaka, @william-gr, @Mariatta
PRs
  • bpo-33625: Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} #7081
    		•

    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 2018-09-07.20:18:16.212>
created_at = <Date 2018-05-23.20:03:01.671>
labels = ['extension-modules', 'type-bug', '3.7']
title = 'Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid}'
updated_at = <Date 2018-09-07.21:18:32.493>
user = 'https://github.com/william-gr'

    bugs.python.org fields:

    activity = <Date 2018-09-07.21:18:32.493>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-09-07.20:18:16.212>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2018-05-23.20:03:01.671>
creator = 'wg'
dependencies = []
files = []
hgrepos = []
issue_num = 33625
keywords = ['patch']
message_count = 13.0
messages = ['317445', '317447', '317557', '317577', '317653', '318797', '319448', '319449', '319478', '319602', '324733', '324734', '324796']
nosy_count = 6.0
nosy_names = ['vstinner', 'christian.heimes', 'ned.deily', 'serhiy.storchaka', 'wg', 'Mariatta']
pr_nums = ['7081']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue33625'
versions = ['Python 3.7']
    @william-gr
    Copy link
    Mannequin Author

    william-gr mannequin commented May 23, 2018

    Hello,

    Currently the GIL is not disabled when calling pwd.getpwnam nor pwd.getpwuid.

    It could be the C library call may take some time for completion, especially when using third-party modules on the system (nss-ldap, nss-pgsql, sss, etc).

    Disabling GIL before calling them makes sure other threads can run in the meantime.

    @william-gr william-gr mannequin added the extension-modules C modules in the Modules dir label May 23, 2018
    @tiran
    Copy link
    Member

    tiran commented May 23, 2018

    Since your patch is a bug fix, it can be back-ported to 2.7 and 3.6/3.7.

    @tiran tiran added 3.7 (EOL) end of life 3.8 only security fixes type-bug An unexpected behavior, bug, or error labels May 23, 2018
    @serhiy-storchaka
    Copy link
    Member

    Functions getpwnam() and getpwuid() are not reentrant. This is not a problem if their use is guarded with GIL and extensions don't call them directly. But if release GIL, we should use reentrant getpwnam_r() and getpwuid_r() instead. There may be an open issue for this, but I can't find it now.

    If getpwnam_r() and getpwuid_r() are not available (are there such modern platforms?) we should use getpwnam() and getpwuid() without releasing GIL.

    @william-gr
    Copy link
    Mannequin Author

    william-gr mannequin commented May 24, 2018

    I have updated the PR to used the re-entrant versions.

    Let me know what you guys think. Thanks!

    @william-gr william-gr mannequin changed the title Disable GIL on getpwnam and getpwuid Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} May 24, 2018
    @Mariatta
    Copy link
    Member

    In the future please use gender-neutral words such as "everyone", "people", or
    "folks" instead of "guys". Thanks.

    @ned-deily
    Copy link
    Member

    With the updated PR that uses reentrant system functions if available, this now seems like a pretty big change to be adding to older maintenance releases, especially since it seems like it would be primarily a performance enhancement and not fixing a "repeatable" bug. I don't think we should backport this to 3.6 or 2.7. If we can get it in prior to 3.7.0rc1, I'd be kinda OK with backporting to 3.7. Sound OK?

    @serhiy-storchaka
    Copy link
    Member

    I think this change is too large for bugfix. It is a performance enhancement, but doing it right needs non-trivial rewriting of the code.

    @vstinner
    Copy link
    Member

    For a recent example of change releasing the GIL, see bpo-32186 which has been backported up to 2.7.

    @ned-deily
    Copy link
    Member

    For a recent example of change releasing the GIL, see bpo-32186 which has been backported up to 2.7.

    Playing Devil's Advocate here: yes, but that was a far simpler and less extensive change. bpo-32186 did not change configure.ac and pyconfig.h.in and I suspect that the impact of the old behavior that bpo-32186 was far more wide spread than that of bpo-33625 (stating files on a NFS file system versus doing getpwnam/getpwuid's). Also when Christian made his comment about a bug fix, the proposed PR was much simpler in scope.

    I am not saying that we definitely should not backport to 3.7 but I don't think it is an automatic call as the PR now stands. In any case, we should first get the fix into master and get some exposure there before deciding whether to backport.

    @vstinner
    Copy link
    Member

    More data to decide if the change should be backported or not: bpo-32186 (Release the GIL during lseek and fstat) has been backported to Python 2.7, but then cffi started to crash:
    https://bugzilla.redhat.com/show_bug.cgi?id=1561170#c28

    At the end, it's a bug in cffi, there was a race condition in cffi. But still, the backport *indirectly* caused a crash.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 7, 2018

    New changeset 23e65b2 by Victor Stinner (William Grzybowski) in branch 'master':
    bpo-33625: Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} (GH-7081)
    23e65b2

    @vstinner
    Copy link
    Member

    vstinner commented Sep 7, 2018

    It has been decided to not touch pwd.getpwall() nor grp.getgrall(): #7081

    @serhiy-storchaka serhiy-storchaka removed the 3.8 only security fixes label Sep 7, 2018
    @vstinner
    Copy link
    Member

    vstinner commented Sep 7, 2018

    I was waiting for the encoding fix to close this issue, but bpo-34604 has been created.

    @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
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @vstinner @tiran @serhiy-storchaka @ned-deily @Mariatta