bpo-33625: Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} #7081

william-gr · 2018-05-23T20:03:58Z

Especially when using more complex nss modules the call might that an
unknown amount of time depending on the service in question.
This makes sures others threads are not blocked waiting the call to
finish.

https://bugs.python.org/issue33625

tiran · 2018-05-23T20:07:24Z

LGTM, thanks!

Please use the blurb tool to add a news entry.

william-gr · 2018-05-23T20:10:35Z

@tiran NEWS added, let me know if thats not to your liking, its my first time doing this ; )

Thanks!

serhiy-storchaka

What about getpwall()? And is it worth to release GIL in the grp and the spwd modules?

serhiy-storchaka · 2018-05-23T20:17:11Z

Misc/NEWS.d/next/Library/2018-05-23-17-07-54.bpo-33625.nzQgD8.rst

@@ -0,0 +1,2 @@
+Release GIL on `pwd.getpwnam` and `pwd.getpwuid`. Patch by William


Sphinx complains about using the default role. Write as :func:`pwd.getpwnam` or ``pwd.getpwnam()``.

william-gr · 2018-05-23T20:24:28Z

@serhiy-storchaka Regarding the getpwall() I thought about it but I wondered about the impact of releasing GIL on every loop iteration. I am not familar with cpython code, would that be bad?

Regarding the other modules, I would think so. Do you want me to do that in this same PR?

serhiy-storchaka · 2018-05-24T05:50:25Z

You are right, and switching a thread between setpwent() and getpwent() is not safe. We would need to add a special global lock for getpwall(). Yet one example when GIL makes the code safer and simpler.

tiran · 2018-05-24T09:41:36Z

Can you either fix getgrgid and getgrnam in this PR or open up another PR. Don't bother about spwd. The module shouldn't be used any more.

william-gr · 2018-05-24T12:19:28Z

@serhiy-storchaka You are absolutely right about re-entrant functions. However I am not sure whats the best way to check if they are available in the system in python project. Should I add a check in configure.ac for the existence of each of them? HAVE_GETPWNAM_R, HAVE_GETPWUID_R ?

william-gr · 2018-05-24T14:23:39Z

I have changed grp module and used the re-entrant versions of them. Let me know what you think. Thanks!

william-gr · 2018-05-24T14:44:12Z

I am now handling ERANGE of _r functions, will update patch soon.

william-gr · 2018-05-24T15:17:00Z

Updated handling ERANGE, let me know how that looks and/or any suggestions/changes.

Thank you again.

william-gr · 2018-05-24T15:58:22Z

Windows-PR seems to be failing but I dont know why, I cant find logs in the link. How should I proceed?

serhiy-storchaka · 2018-05-24T18:12:37Z

Modules/grpmodule.c

+    int status, bufsize = NSS_BUFLEN_GROUP;
+    struct group *grpbuf = NULL;
+
+    p = malloc(sizeof(struct group));


This is a constant size buffer. It is better to use a local variable for it.

struct group grp; ... status = getgrgid_r(gid, &grp, buf, bufsize, &p);

serhiy-storchaka · 2018-05-24T18:13:05Z

Modules/grpmodule.c

+    struct group *grpbuf = NULL;
+
+    p = malloc(sizeof(struct group));
+    buf = malloc(bufsize);


Use PyMem_RawMalloc().

serhiy-storchaka · 2018-05-24T18:15:58Z

Modules/grpmodule.c

+
+    do {
+        status = getgrgid_r(gid, p, buf, bufsize, &grpbuf);
+        if(grpbuf == NULL && status == ERANGE) {


Please add a space between if and ( for conforming PEP 7.

serhiy-storchaka · 2018-05-24T18:19:44Z

Modules/grpmodule.c

+    buf = malloc(bufsize);
+
+    do {
+        status = getgrgid_r(gid, p, buf, bufsize, &grpbuf);


Use p instead of grpbuf.

serhiy-storchaka · 2018-05-24T18:25:34Z

Modules/grpmodule.c

+
+    do {
+        status = getgrgid_r(gid, p, buf, bufsize, &grpbuf);
+        if(grpbuf == NULL && status == ERANGE) {


This condition is doubled in the while below. It would be better to write:

if (grpbuf != NULL || status != ERANGE) { break; }

and use while (1).

serhiy-storchaka · 2018-05-24T18:26:49Z

Modules/grpmodule.c

+    do {
+        status = getgrgid_r(gid, p, buf, bufsize, &grpbuf);
+        if(grpbuf == NULL && status == ERANGE) {
+            free(buf);


It could be more efficient to use realloc.

serhiy-storchaka · 2018-05-24T18:27:26Z

Modules/grpmodule.c

+        status = getgrgid_r(gid, p, buf, bufsize, &grpbuf);
+        if(grpbuf == NULL && status == ERANGE) {
+            free(buf);
+            if((bufsize << 1) > (NSS_BUFLEN_GROUP << 10)) {


What is this check for?

This is to make sure we are not allocating too big of a buffer (its a maximum cap)

william-gr · 2018-05-24T18:58:01Z

@serhiy-storchaka thanks for the review. Let me know if I have addressed them to your liking.

william-gr · 2018-05-25T12:55:35Z

Is there a way I can trigger a new macOS build? The failure does not seem related to the commit.

bedevere-bot · 2018-09-06T14:43:56Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner

New review. My comments apply to the all modified functions.

vstinner · 2018-09-06T14:49:04Z

Modules/grpmodule.c

+#endif
+    if (p == NULL) {
+        if (buf != NULL) {
+            PyMem_RawFree(buf);


PyMem_RawFree(NULL) is valid (does nothing), the if() is useless.

vstinner · 2018-09-06T14:51:10Z

Modules/grpmodule.c

+            break;
+        }
+        bufsize <<= 1;
+        p = PyMem_RawRealloc(buf, bufsize);


It's strange to reuse the "struct group *p;" variable to reallocate the "char *buf = NULL;" variable: C types are not the same!? I wold prefer to see a different variable (ex: "buf2").

vstinner · 2018-09-06T14:53:55Z

Modules/grpmodule.c

+
+    bufsize = sysconf(_SC_GETGR_R_SIZE_MAX);
+    if (bufsize == -1) {
+        bufsize = 1024;


Would you mind to add a "#define DEFAULT_BUFFER_SIZE 1024" at top level, rather than using an hardcoded constant here?

vstinner · 2018-09-06T14:54:25Z

Modules/grpmodule.c

+    if (p == NULL) {
+        if (nomem == 1) {
+            PyErr_NoMemory();
+        } else {


nitpick: the PEP 7 requires

} else {

vstinner · 2018-09-06T14:55:39Z

Modules/pwdmodule.c

+        buf = (char *) p;
+    }
+
+    if (status != 0) {


I would prefer to move this just after the get...() call.

bedevere-bot · 2018-09-06T14:57:00Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

william-gr · 2018-09-06T15:55:56Z

I have made the requested changes; please review again

Thank you

bedevere-bot · 2018-09-06T15:56:01Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner

Ah! Honestly, the new PR is way better than the first version! I was going to merge it, when I spotted another issue: you don't check if PyMem_Malloc() fails. IMHO it's not a good idea to call get*() functions with buf=NULL. Maybe the function fails, maybe you get a crash... I would prefer to avoid the risk of crash :-) I proposed to move code which allocates the memory to avoid redundancy and to make sure that get*() are never called with buf=NULL.

By the way, I also proposed to fix a mojibake (encoding) issue while we are on these functions.

Maybe the mojibake issue can be fixed in a separated PR, since I don't think that we are going to backport this one to 3.7 and older. So maybe wait until this PR is merged, and then write a second PR to fix the mojibake issue, and we can easily backport the second PR to all supported branches.

vstinner · 2018-09-07T07:24:20Z

Modules/grpmodule.c

+            PyErr_NoMemory();
+        }
+        else {
+            PyErr_Format(PyExc_KeyError, "getgrnam(): name not found: %s", name_chars);


Hum, I see a bug which is not related to your change, but while we modify these functions, it would be nice to fix it.

name_chars is encoded to the filesystem encoding, whereas %s decodes it from UTF-8. If the filesystem encoding is not UTF-8, you get mojibake.

The fix is simple: use %S format and pass name: name is always a Unicode string.

If name is always a Unicode string, you can use %U.

It may be worth to use %R for the case if the name contains invisible characters or trailing whitespaces.

See https://bugs.python.org/issue34604 :-)

vstinner · 2018-09-07T07:26:00Z

Modules/pwdmodule.c

+        }
+        else {
+            PyErr_Format(PyExc_KeyError,
+                         "getpwnam(): name not found: %s", name);


Same comment: please use the %S format here, but if you modify this function, would you mind to also fix the name of the parameter? Rename "arg" to "name", as in Doc/library/pwd.rst. Maybe rename the char* name to name_chars, as in grp_getgrnam_impl().

vstinner · 2018-09-07T07:28:47Z

Modules/grpmodule.c

+    if (bufsize == -1) {
+        bufsize = DEFAULT_BUFFER_SIZE;
+    }
+    buf = PyMem_RawMalloc(bufsize);


Hum, wait, you don't check if PyMem_Malloc() failed here? I'm not sure that it's safe to call get*() functions with buf=NULL.

I have a proposition. Move the memory allocation at the start of the loop, and always use PyMem_Realloc(). PyMem_Realloc(buf, bufsize) behaves as PyMem_Malloc(). Pseudo-code:

/* PyMem_Malloc code removed from here */ while (1) { buf2 = PyMem_RawRealloc(buf, bufsize); if (buf2 == NULL) { nomem = 1; break; } buf = buf2; /* buf cannot be NULL from this point */ (...) /* no more PyMem_Realloc code here neither */ }

(I don't propose to add these comments, it's just to explain my idea ;-))

bedevere-bot · 2018-09-07T07:34:12Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@vstinner

Suggested by: @vstinner

william-gr · 2018-09-07T10:57:28Z

I have made the requested changes; please review again

Thanks for pointing that out and your patience with the reviews. I am glad you pointed the Malloc issue, I also found other problem with latest Realloc change I had made.

I will take your advise and create another PR for mojibake.

bedevere-bot · 2018-09-07T10:57:30Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner · 2018-09-07T12:06:48Z

I merged your PR, thanks!

Would you mind to write a second PR to fix the encoding issue that I spootted? I explained how to fix it (use %S format).

the-knights-who-say-ni added the CLA signed label May 23, 2018

bedevere-bot added the awaiting review label May 23, 2018

tiran added needs backport to 3.6 labels May 23, 2018

william-gr force-pushed the pwd-release-gil branch from 9ecf8a3 to 122536d Compare May 23, 2018 20:08

william-gr force-pushed the pwd-release-gil branch from 122536d to ef18509 Compare May 23, 2018 20:15

serhiy-storchaka reviewed May 23, 2018

View reviewed changes

william-gr force-pushed the pwd-release-gil branch from ef18509 to f8f90e2 Compare May 24, 2018 14:20

william-gr changed the title ~~bpo-33625: Release GIL for getpwnam and getpwuid~~ bpo-33625: Release GIL for grp.getgr(nam,gid} and pwd.getpw{nam,uid} May 24, 2018

william-gr changed the title ~~bpo-33625: Release GIL for grp.getgr(nam,gid} and pwd.getpw{nam,uid}~~ bpo-33625: Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} May 24, 2018

william-gr force-pushed the pwd-release-gil branch from f8f90e2 to 4722749 Compare May 24, 2018 14:33

william-gr force-pushed the pwd-release-gil branch 2 times, most recently from cc6368f to 1e68811 Compare May 24, 2018 15:16

serhiy-storchaka reviewed May 24, 2018

View reviewed changes

william-gr force-pushed the pwd-release-gil branch 3 times, most recently from 7702a5f to 84031ff Compare May 24, 2018 19:53

vstinner requested changes Sep 6, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Sep 6, 2018

william-gr force-pushed the pwd-release-gil branch from 5c47292 to 0da8c19 Compare September 6, 2018 15:54

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 6, 2018

Address reviews from @vstinner

874339e

william-gr force-pushed the pwd-release-gil branch from 0da8c19 to 874339e Compare September 6, 2018 16:44

vstinner requested changes Sep 7, 2018

View reviewed changes

bedevere-bot removed the awaiting change review label Sep 7, 2018

bedevere-bot added the awaiting changes label Sep 7, 2018

Move Realloc and remove Malloc so we always check if it succeeded

b02311e

Suggested by: @vstinner

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 7, 2018

vstinner approved these changes Sep 7, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 7, 2018

vstinner merged commit 23e65b2 into python:master Sep 7, 2018

bedevere-bot removed the awaiting merge label Sep 7, 2018

william-gr deleted the pwd-release-gil branch September 7, 2018 13:05

william-gr mentioned this pull request Sep 7, 2018

bpo-34604: Fix possible mojibake in pwd.getpwnam and grp.getgrnam #9098

Merged

		@@ -0,0 +1,2 @@
		Release GIL on `pwd.getpwnam` and `pwd.getpwuid`. Patch by William

bpo-33625: Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} #7081

bpo-33625: Release GIL for grp.getgr{nam,gid} and pwd.getpw{nam,uid} #7081

Conversation

william-gr commented May 23, 2018 • edited by bedevere-bot Loading

tiran commented May 23, 2018

william-gr commented May 23, 2018

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

william-gr commented May 23, 2018

serhiy-storchaka commented May 24, 2018

tiran commented May 24, 2018

william-gr commented May 24, 2018

william-gr commented May 24, 2018

william-gr commented May 24, 2018

william-gr commented May 24, 2018

william-gr commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

william-gr commented May 24, 2018

william-gr commented May 25, 2018

bedevere-bot commented Sep 6, 2018

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 6, 2018

william-gr commented Sep 6, 2018

bedevere-bot commented Sep 6, 2018

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 7, 2018

william-gr commented Sep 7, 2018

bedevere-bot commented Sep 7, 2018

vstinner commented Sep 7, 2018

william-gr commented May 23, 2018 •

edited by bedevere-bot

Loading