bpo-31938: Convert selectmodule.c to Argument Clinic #4265

taleinat · 2017-11-03T21:58:13Z

This is a complete Argument Clinic conversion of Modules/selectmodule.c.

The initial commit on this branch is the mostly fixed application of the latest patch on bpo-20182. Later changes were each done in a separate commit to facilitate review.

Minor note: Some of the early commits on the branch don't compile successfully.

https://bugs.python.org/issue31938

Not in working condition, things need to be moved around to work with generation of AC code in a separate file.

This is required to be able to include the generated AC code from clinic/selectmodule.c.h, which must appear before these definitions but after all typedefs and type declarations.

The converter was copied from Modules/posixmodule.c, but changed to check whether fd == -1 rather than fd < 0 as an error condition from calling PyOjbect_AsFileDescriptor(). Note: AC output in Modules/clinic/selectmodule.c.h was manually modified in this commit to get the build working. This may be due to an AC bug.

Note: AC output in Modules/clinic/selectmodule.c.h was manually modified in this commit to get the build working. This may be due to an AC bug.

…rter

Also change the doc-string of epoll.poll: * use AC's per-argument docs to describe the arguments * revert an addition describing the return value Note: AC output in Modules/clinic/selectmodule.c.h was manually modified in this commit to get the build working. This may be due to an AC bug.

serhiy-storchaka · 2017-11-03T22:12:10Z

Modules/selectmodule.c

+
+    fd: fildes
+      either an integer, or an object with a fileno() method returning an int
+    eventmask: object(converter="ushort_converter", type="unsigned short", c_default="POLLIN | POLLOUT | POLLPRI") = POLLIN | POLLOUT | POLLPRI


Good opportunity to register the ushort converter.

Please keep flags in this order: POLLIN | POLLPRI | POLLOUT. It seems like POLLPRI is more related to POLLIN, than POLLOUT. See https://bugs.python.org/issue30844 :-)

serhiy-storchaka · 2017-11-03T22:15:34Z

Modules/selectmodule.c

-    if (!PyArg_ParseTuple(args, "|O:poll", &timeout_obj)) {
-        return NULL;
-    }
-
    if (timeout_obj != NULL && timeout_obj != Py_None) {


timeout_obj can't be NULL.

serhiy-storchaka · 2017-11-03T22:20:15Z

Modules/selectmodule.c

-    if (!PyArg_ParseTuple(args, "|O:poll", &timeout_obj)) {
-        return NULL;
-    }
-
    /* Check values for timeout */
    if (timeout_obj == NULL || timeout_obj == Py_None) {


timeout_obj can't be NULL.

serhiy-storchaka · 2017-11-03T22:23:52Z

Modules/selectmodule.c

+@classmethod
+select.epoll.__new__
+
+    sizehint: int(c_default="FD_SETSIZE - 1") = -1


Negative sizehint is an error.

Should it be an error? The tests only check that -2 should raise ValueError, not -1. The existing doc-string mentions that -1 should mean the default size, though it currently doesn't.

Perhaps we should fix the code to special-case -1? Currently the only way to get the default is to not specify sizehint, but that's one type of inconsistency AC aims to remove.

See the check at line 1304. The current code treats -1 as invalid value. Argument Clinic conversion shouldn't change semantic (too much). If there is a bug in the current code, fix it in a separate issue, so that the fix can be backported.

This may not actually matter much. From the docs: "sizehint and flags are deprecated and completely ignored." Looking at the code, this is true for flags, but sizehint is actually passed to epoll_create if HAVE_EPOLL_CREATE1 isn't defined.

So there are two problems right now:

The current doc-string is simply wrong: "sizehint must be a positive integer or -1 for the default size"

Since -1 is actually invalid, there's currently no value that can be used in the doc-string to describe the default for sizehint, which is slightly bad for AC conversion.

Fixing the doc-string to describe the actual behavior seems like the way to go. I'm just not sure what that is regarding sizehint: is it actually ignored? I.e. are the docs wrong on this?

I'm really unsure what to do regarding the default value for sizehint shown in the doc-string.

@serhiy-storchaka Could you help decide how to resolve this point?

I'm summarizing in hope to move forward with this.

The default for sizehint should ideally be FD_SETSIZE - 1, but FD_SETSIZE isn't exposed at the Python level. The current behavior sets sizehint to FD_SETSIZE - 1 if not supplied, but that's a type of inconsistency that AC aims to remove.

-1 is an invalid value, therefore it should probably not be used as a default value since that would change the existing semantics too much.

The docs say that sizehint is deprecated and ignored, in which case this discussion would be moot. However in the code sizehint is still passed to epoll_create() if epoll_create1() is not available:

#ifdef HAVE_EPOLL_CREATE1 self->epfd = epoll_create1(EPOLL_CLOEXEC); #else self->epfd = epoll_create(sizehint); #endif

-1 was valid before 2fb9ae9 (but 0 was not valid). I'm not sure this change is intentional. It is better to open a separate issue for this.

@serhiy-storchaka, I'll open a new issue. In the meantime, what do we do with the default value of sizehint in this branch?

Depending on the results of that issue. Likely -1 will be the default value.

Created bpo-32568.

serhiy-storchaka · 2017-11-03T22:27:27Z

Modules/selectmodule.c


-    return pyepoll_internal_ctl(self->epfd, EPOLL_CTL_DEL, pfd, 0);
-}
+    timeout as timeout_obj: object(c_default="Py_None") = -1.0


Why not use None as a default?

The doc-string mentions, regarding timeout, "-1 makes poll wait indefinitely" but doesn't mention None, though None actually does the same. Given this, I thought documenting the default as -1 was clearer.

Perhaps we should improve the doc-string, mentioning that either None or -1 mean "wait indefinitely", and then change the default to None?

Do what looks better to you.

I'll improve the doc-string and change the default to None.

serhiy-storchaka · 2017-11-03T22:31:58Z

Modules/selectmodule.c

@@ -2158,8 +2087,8 @@ kqueue_queue_control(kqueue_queue_Object *self, PyObject *args)
        ptimeoutspec = &timeoutspec;
    }

-    if (ch != NULL && ch != Py_None) {
-        seq = PySequence_Fast(ch, "changelist is not iterable");
+    if (changelist != NULL && changelist != Py_None) {


otimeout and changelist can't be NULL.

vstinner

Thank you for this nice PR. While I may have other comments later, and @serhiy-storchaka already added comments, I like how the new code looks like ;-)

vstinner · 2017-11-03T23:08:35Z

Modules/selectmodule.c

@@ -702,39 +748,6 @@ poll_dealloc(pollObject *self)
    PyObject_Del(self);
 }

-static PyTypeObject poll_Type = {


Would it be possible to not move this code, to group all code related to poll at the same place, as done currently.

Currently we must either move all method and type definitions towards the end of the file, or move all typedefs and type declarations to the beginning. This is because #include "clinic/selectmodule.c.h" must come between them.

On bpo-20180 (msg304976), regarding itertools, @serhiy-storchaka mentioned preference for moving the method and type definitions to the end rather than the alternative. Therefore I did the same here.

This is not a preference, this is just an option. Since methods lists and type object initializers could be generated by Argument Clinic in future, I think it is easier to move them.

vstinner · 2017-11-03T23:11:27Z

Modules/selectmodule.c

+
+    fd: fildes
+      either an integer, or an object with a fileno() method returning an int
+    eventmask: object(converter="ushort_converter", type="unsigned short", c_default="POLLIN | POLLOUT | POLLPRI") = POLLIN | POLLOUT | POLLPRI


Please keep flags in this order: POLLIN | POLLPRI | POLLOUT. It seems like POLLPRI is more related to POLLIN, than POLLOUT. See https://bugs.python.org/issue30844 :-)

vstinner · 2017-11-03T23:34:48Z

Modules/selectmodule.c

-      int.\n\
-events -- an optional bitmask describing the type of events to check for");
+/*[clinic input]
+select.poll.register


Please use "-> NoneType". Even if currently it seems like it has no effect, it will be used in signature once https://bugs.python.org/issue31939 will be implemented.

That syntax is for defining AC return converters, not return type annotations, so it could have unwanted effects.

Specifically, the Argument Clinic How-To currently states:

(There’s also an experimental NoneType converter, which lets you return Py_None on success or NULL on failure, without having to increment the reference count on Py_None. I’m not sure it adds enough clarity to be worth using.)

vstinner · 2017-11-03T23:35:12Z

Modules/selectmodule.c

-static PyObject*
-devpoll_fileno(devpollObject *self)
+/*[clinic input]
+select.devpoll.fileno


Please try to use "-> int" return converter. Same comment for other fileno() methods.

Regarding devpoll.fileno(), due to the use of devpoll_err_closed() internally, using -> int or -> long actually complicates the code. The same goes for epoll.fileno() and kqueue.fileno() as well.

This will complicate the code. select_devpoll_fileno_impl would be need to convert NULL returned by devpoll_err_closed() to -1. I think it is better to keep the current code and add the converter in a separate issue if you with.

vstinner

This PR is just giant. I would suggest to split it into multiple smaller PRs. For example, why not starting by converting only select()?

vstinner · 2018-06-22T11:28:24Z

Modules/selectmodule.c

+    int fd;
+    int *pointer = (int *)p;
+    fd = PyObject_AsFileDescriptor(o);
+    if (fd == -1)


PEP 7: add { ... }.

vstinner · 2018-06-22T11:28:41Z

Modules/selectmodule.c

+{
+    int fd;
+    int *pointer = (int *)p;
+    fd = PyObject_AsFileDescriptor(o);


nitpick: you can write "int fd = ...;" (single line)

vstinner · 2018-06-22T11:29:32Z

Modules/selectmodule.c

+
+*** IMPORTANT NOTICE ***
+On Windows, only sockets are supported; on Unix, all file
+descriptors can be used.


used => supported or acceptd.

vstinner · 2018-06-22T11:30:21Z

Modules/selectmodule.c

+Wait until one or more file descriptors are ready for some kind of I/O.
+
+The first three arguments are sequences of file descriptors to be waited for:
+rlist -- wait until ready for reading


I would remove "wait until" since select is non-blocking when using a timeout of 0.

vstinner · 2018-06-22T11:31:05Z

Modules/selectmodule.c

+The first three arguments are sequences of file descriptors to be waited for:
+rlist -- wait until ready for reading
+wlist -- wait until ready for writing
+xlist -- wait for an ``exceptional condition''


the prevent encoding issues on legacy terminals, I suggest to use more common and ASCII quote: '...'

See also https://bugs.python.org/issue28710 .

vstinner · 2018-06-22T11:33:16Z

Modules/selectmodule.c

+xlist -- wait for an ``exceptional condition''
+If only one kind of condition is required, pass [] for the other lists.
+A file descriptor is either a socket or file object, or a small integer
+gotten from a fileno() method call on one of those.


I'm not sure if it's worth it to mention it, but on Windows, technically select() expects a list of socket handles, not file descriptors. On Windows, socket handles and file descriptors are two distinct namespaces.

vstinner · 2018-06-22T11:36:39Z

Modules/selectmodule.c

-        efdlist = set2list(&efdset, efd2obj);
+        rlist = set2list(&ifdset, rfd2obj);
+        wlist = set2list(&ofdset, wfd2obj);
+        xlist = set2list(&efdset, efd2obj);


I would prefer "elist" to be consisten with "efdset" and "efd2obj".

taleinat · 2018-06-22T11:54:04Z

This PR is just giant. I would suggest to split it into multiple smaller PRs. For example, why not starting by converting only select()?

I can understand why you suggest this. I'm discouraged by the amount of work required in addition to that already invested here. I'm further discouraged since I've done exactly what you suggest for the AC conversion of itertools, beginning with converting only groupby(), but that has been completely ignored so far (PR #4170).

vstinner · 2018-06-22T17:07:10Z

I've done exactly what you suggest for the AC conversion of itertools, beginning with converting only groupby(), but that has been completely ignored so far (PR #4170).

I like AC and want to convert everything to AC, but I have limited bandwidth to review such AC. Please don't hesitate to ping me frequently until I reviewed all of them :-)

serhiy-storchaka · 2018-06-22T17:50:28Z

Making all changes with a single PR LGTM. This is unavoidable when convert to AC such module. Let me to recall where we stopped last time. I guess there were just minor problems which should be resolved.

taleinat · 2018-06-22T18:19:36Z

@serhiy-storchaka

Let me to recall where we stopped last time.

I've got a patch ready which addresses nearly all of those issues. If you're interested I'll get it done and push it to this branch. I suggest that you wait until that's done before reviewing again.

taleinat · 2018-06-22T18:21:40Z

@vstinner

I like AC and want to convert everything to AC, but I have limited bandwidth to review such AC. Please don't hesitate to ping me frequently until I reviewed all of them :-)

I greatly appreciate your bandwidth, it's obvious you spend a lot of time and effort to address so many different issues! I realize that AC is lower priority than most things and accept that. I'll work on moving my AC patches one at a time, in hopes of getting things moving in smaller steps will require less ongoing commitment from other core devs.

taleinat · 2018-06-22T18:24:23Z

@vstinner, AFAICT all of your comments in the last batch are regarding code and comments which I mostly moved around rather than actually wrote or edited. I'd rather make such changes after the AC conversion if you don't mind. I'd gladly collect your comments here and prepare another PR with just those improvements, after this is done with.

# Conflicts: # Modules/selectmodule.c

taleinat · 2018-06-22T19:54:35Z

@serhiy-storchaka, I've fixed all of the outstanding issues brought up here that it was clear should be fixed. I think this is ready for another review!

I've left the epoll sizehint behavior as-is, i.e. this patch doesn't change the existing behavior, questionable though it may be. We are addressing that on the dedicated issue bpo-32568.

I've also not addressed the comments from @vstinner's latest review, since they are all about things I haven't actually changed here.

vadmium · 2018-06-23T08:55:07Z

Modules/selectmodule.c

@@ -875,13 +896,20 @@ devpoll_unregister(devpollObject *self, PyObject *o)
    Py_RETURN_NONE;
 }

-PyDoc_STRVAR(devpoll_poll_doc,
-"poll( [timeout] ) -> list of (fd, event) 2-tuples\n\n\


The hint about the structure of the return value is lost. Same for epoll.poll.

Good catch! I'd addressed this for poll.poll() but not devpoll.poll() and epoll.poll(). Fixed!

serhiy-storchaka · 2018-06-25T13:25:35Z

Modules/selectmodule.c

+The first three arguments are sequences of file descriptors to be waited for:
+rlist -- wait until ready for reading
+wlist -- wait until ready for writing
+xlist -- wait for an ``exceptional condition''


See also https://bugs.python.org/issue28710 .

serhiy-storchaka · 2018-06-25T13:49:11Z

Modules/selectmodule.c

+/*[clinic input]
+select.devpoll.poll
+
+    timeout as timeout_obj: object = None


Should be positional-only.

taleinat · 2018-06-25T18:30:12Z

@serhiy-storchaka, I think this is ready for another review!

I've addressed your latest two comments and fixed a couple of bugs related to select.kqueue (now all tests are passing).

# Conflicts: # Modules/selectmodule.c

taleinat added 9 commits November 3, 2017 23:48

bpo-20182: initial application of issue20182.selectmodule.v2.patch

391f243

Not in working condition, things need to be moved around to work with generation of AC code in a separate file.

bpo-20182: AC convert new select.kqueue and fix missing summary lines

3423836

bpo-20182: selectmodule.c AC: move type and method defintions to bottom

778d240

This is required to be able to include the generated AC code from clinic/selectmodule.c.h, which must appear before these definitions but after all typedefs and type declarations.

bpo-20182: selectmodule.c AC: convert select.devpoll to AC

fc964f0

Note: AC output in Modules/clinic/selectmodule.c.h was manually modified in this commit to get the build working. This may be due to an AC bug.

bpo-20182: selectmodule.c AC: fix and standardize use of ushort_conve…

b415d95

…rter

bpo-20182: selectmodule.c AC: fix two compilation errors in epoll

b91b2db

bpo-20182: selectmodule.c AC: re-run clinic.py after its recent bugfix

7939e70

taleinat added the skip news label Nov 3, 2017

taleinat requested review from vstinner and serhiy-storchaka November 3, 2017 21:58

the-knights-who-say-ni added the CLA signed label Nov 3, 2017

bedevere-bot added the awaiting merge label Nov 3, 2017

serhiy-storchaka reviewed Nov 3, 2017

View reviewed changes

vstinner reviewed Nov 3, 2017

View reviewed changes

vstinner reviewed Jun 22, 2018

View reviewed changes

taleinat added 2 commits June 22, 2018 22:02

bpo-31938: address remaining CR issues

7de2e5d

Merge branch 'master' into bpo-31938_AC_selectmodule

15f4b0f

# Conflicts: # Modules/selectmodule.c

vadmium reviewed Jun 23, 2018

View reviewed changes

taleinat added 2 commits June 24, 2018 13:40

bpo-31938: describe return values of devpoll.poll() and epoll.poll()

1173711

bpo-31938: re-run clinic.py on Modules/selectmodule.c

65d2f0d

serhiy-storchaka reviewed Jun 25, 2018

View reviewed changes

taleinat added 5 commits June 25, 2018 20:54

bpo-31938: fix a bug in select_kqueue_fromfd_impl

0b81db5

bpo-31938: minor doc-string formatting fix

d5a8516

bpo-31938: fix select.devpoll.poll() timeout param to positional only

cff3113

bpo-31938: re-run clinic.py on Modules/selectmodule.c (again)

aa421fb

bpo-31938: fix broken function reference in kqueue_queue_Type

1432332

Merge branch 'master' into bpo-31938_AC_selectmodule

1474605

# Conflicts: # Modules/selectmodule.c

serhiy-storchaka approved these changes Jun 30, 2018

View reviewed changes

taleinat merged commit 6dc57e2 into python:master Jun 30, 2018

bedevere-bot removed the awaiting merge label Jun 30, 2018

bpo-31938: Convert selectmodule.c to Argument Clinic #4265

bpo-31938: Convert selectmodule.c to Argument Clinic #4265

Conversation

taleinat commented Nov 3, 2017 • edited Loading

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

taleinat Nov 4, 2017 • edited Loading

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

taleinat Jan 16, 2018 • edited Loading

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

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taleinat Nov 4, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taleinat commented Jun 22, 2018

vstinner commented Jun 22, 2018

serhiy-storchaka commented Jun 22, 2018

taleinat commented Jun 22, 2018

taleinat commented Jun 22, 2018

taleinat commented Jun 22, 2018

taleinat commented Jun 22, 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

taleinat commented Jun 25, 2018

taleinat commented Nov 3, 2017 •

edited

Loading

taleinat Nov 4, 2017 •

edited

Loading

taleinat Jan 16, 2018 •

edited

Loading

taleinat Nov 4, 2017 •

edited

Loading