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

[william-gr]

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]

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.

[tiran]

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

[serhiy-storchaka]

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]

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

Let me know what you guys think. Thanks!

[Mariatta]

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

[ned-deily]

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]

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]

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

[ned-deily]

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]

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]

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]

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

[vstinner]

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