-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-42085: Introduce dedicated entry in PyAsyncMethods for sending values #22780
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. +1 from me. This is another feature I wished we added right when we approved PEP 492.
On the PR: please add the docs / news / what's new.
Include/cpython/object.h
Outdated
@@ -167,10 +167,23 @@ typedef struct { | |||
objobjargproc mp_ass_subscript; | |||
} PyMappingMethods; | |||
|
|||
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030A0000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cpython/object.h
is only imported when Py_LIMITED_API
is not defined, so this #if
does not make sense. Also, moving the declaration of PySendResult
here removes it from the limited API. It actually break the limited API because importing abstract.h
with the limited API will be error.
Modules/_asynciomodule.c
Outdated
static PyObject * | ||
FutureIter_iternext(futureiterobject *it) | ||
static PySendResult | ||
FutureIter_am_send(futureiterobject *it, PyObject *arg, PyObject **result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is arg
ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, there is a comment in FutureIter_send
, I can copy it here as well:
/* Future.__iter__ doesn't care about values that are pushed to the
* generator, it just returns self.result().
*/
@vladima Please add the docs for the new C API. And I think this is ready. |
@@ -1031,7 +1011,8 @@ static PyMethodDef coro_methods[] = { | |||
static PyAsyncMethods coro_as_async = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, with this PR regular generators no longer benefit from the faster PyIter_Send
. I think we should implement am_send
for regular generators too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we need to address https://github.com/python/cpython/pull/22780/files#r508796665 too.
typedef struct { | ||
unaryfunc am_await; | ||
unaryfunc am_aiter; | ||
unaryfunc am_anext; | ||
sendfunc am_send; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not we add corresponding Py_TPFLAGS_HAVE_
flag for binary compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeslots.h
should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not we add corresponding Py_TPFLAGS_HAVE_ flag for binary compatibility?
Do we support extensions compiled for higher versions of Python in lower versions of Python? If I compile something for 3.10 I wouldn't expect it to work in 3.9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we don't need a Py_TPFLAGS_HAVE_
flag and I don't remember adding one for the as_async
slot when we implemented PEP 492.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you compile something for 3.9 it is expected to work in 3.10 and do not read uninitialized slot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps there is a bug in the PEP 492 implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I see now, thanks for explaining! Yeah, in this case we do need a flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps there is a bug in the PEP 492 implementation.
Well, I don't think we need to do anything now that as_async
has been there since 3.5. But looking back, maybe we should have added a flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used an existing slot for PEP-492, so the ABI did not change.
I don't think we necessarily need a new flag. See https://bugs.python.org/issue32388
Objects/typeobject.c
Outdated
/* Consistency check for Py_TPFLAGS_HAVE_SEND - flag requires | ||
* type->tp_as_async->am_send to be present. | ||
*/ | ||
if (type->tp_flags & Py_TPFLAGS_HAVE_SEND) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be more specific with the flag name.
if (type->tp_flags & Py_TPFLAGS_HAVE_SEND) { | |
if (type->tp_flags & Py_TPFLAGS_HAVE_AM_SEND) { |
Modules/_asynciomodule.c
Outdated
static PyObject * | ||
FutureIter_iternext(futureiterobject *it) | ||
static PySendResult | ||
FutureIter_am_send(futureiterobject *it, PyObject *Py_UNUSED(arg), PyObject **result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to keep lines under 79 characters
@serhiy-storchaka could you please review again? |
@pablogsal Pablo maybe you can take a quick look at this? |
I can try to give it a look next week if I find some time. |
@scoder @serhiy-storchaka Guys, could you please take another look? I plan to merge this soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the hard work
Alright, I'm going to go ahead and merge this. @serhiy-storchaka and @scoder if you want to further discuss this let's continue on bpo and in follow up PRs. Thanks to everybody for reviews! |
@1st1: Please replace |
As in title: create
am_send
slot inPyAsyncMethods
to send values.Skipping new section since I expect a lot of changes as the result of discussions
https://bugs.python.org/issue42085