classification
Title: Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid}
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mariatta, christian.heimes, ned.deily, serhiy.storchaka, vstinner, wg
Priority: normal Keywords: patch

Created on 2018-05-23 20:03 by wg, last changed 2018-09-07 21:18 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 7081 merged wg, 2018-05-23 20:04
Messages (13)
msg317445 - (view) Author: William Grzybowski (wg) * Date: 2018-05-23 20:03
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.
msg317447 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-05-23 20:09
Since your patch is a bug fix, it can be back-ported to 2.7 and 3.6/3.7.
msg317557 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-05-24 11:30
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.
msg317577 - (view) Author: William Grzybowski (wg) * Date: 2018-05-24 14:24
I have updated the PR to used the re-entrant versions.

Let me know what you guys think. Thanks!
msg317653 - (view) Author: Mariatta Wijaya (Mariatta) * (Python committer) Date: 2018-05-25 01:17
In the future please use gender-neutral words such as "everyone", "people", or
"folks" instead of "guys". Thanks.
msg318797 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-06-06 02:56
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?
msg319448 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-06-13 10:17
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.
msg319449 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-06-13 10:24
For a recent example of change releasing the GIL, see bpo-32186 which has been backported up to 2.7.
msg319478 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-06-13 17:14
> 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.
msg319602 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-06-15 10:41
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.
msg324733 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-07 12:06
New changeset 23e65b25557f957af840cf8fe68e80659ce28629 by Victor Stinner (William Grzybowski) in branch 'master':
bpo-33625: Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} (GH-7081)
https://github.com/python/cpython/commit/23e65b25557f957af840cf8fe68e80659ce28629
msg324734 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-07 12:16
It has been decided to not touch pwd.getpwall() nor grp.getgrall(): https://github.com/python/cpython/pull/7081
msg324796 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-07 21:18
I was waiting for the encoding fix to close this issue, but bpo-34604 has been created.
History
Date User Action Args
2018-09-07 21:18:32vstinnersetmessages: + msg324796
2018-09-07 20:18:16serhiy.storchakasetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: - Python 3.8
2018-09-07 12:16:13vstinnersetmessages: + msg324734
2018-09-07 12:06:24vstinnersetmessages: + msg324733
2018-06-15 10:41:52vstinnersetmessages: + msg319602
2018-06-13 17:14:44ned.deilysetmessages: + msg319478
2018-06-13 10:24:11vstinnersetmessages: + msg319449
2018-06-13 10:17:52serhiy.storchakasetmessages: + msg319448
2018-06-06 09:30:07vstinnersetnosy: + vstinner
2018-06-06 02:56:19ned.deilysetnosy: + ned.deily

messages: + msg318797
versions: - Python 2.7, Python 3.6
2018-05-25 01:17:46Mariattasetnosy: + Mariatta
messages: + msg317653
2018-05-24 14:24:18wgsetmessages: + msg317577
title: Disable GIL on getpwnam and getpwuid -> Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid}
2018-05-24 11:30:55serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg317557
2018-05-23 20:09:07christian.heimessetversions: + Python 2.7, Python 3.7, Python 3.8
nosy: + christian.heimes

messages: + msg317447

type: behavior
2018-05-23 20:04:00wgsetkeywords: + patch
stage: patch review
pull_requests: + pull_request6711
2018-05-23 20:03:01wgcreate