| 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) *  |
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) * |
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 (Mariatta) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2018-09-07 21:18 |
| I was waiting for the encoding fix to close this issue, but bpo-34604 has been created. |
|
|