8000 bpo-42085: Introduce dedicated entry in PyAsyncMethods for sending values by vladima · Pull Request #22780 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 8 commits into from
Nov 10, 2020

Conversation

vladima
Copy link
Contributor
@vladima vladima commented Oct 19, 2020

As in title: create am_send slot in PyAsyncMethods to send values.
Skipping new section since I expect a lot of changes as the result of discussions

https://bugs.python.org/issue42085

Copy link
Member
@1st1 1st1 left a 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.

cc @scoder @serhiy-storchaka

@@ -167,10 +167,23 @@ typedef struct {
objobjargproc mp_ass_subscript;
} PyMappingMethods;

#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030A0000
Copy link
Member

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.

static PyObject *
FutureIter_iternext(futureiterobject *it)
static PySendResult
FutureIter_am_send(futureiterobject *it, PyObject *arg, PyObject **result)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is arg ignored?

Copy link
Contributor Author

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 vladima closed this Oct 19, 2020
@vladima vladima reopened this Oct 19, 2020
@1st1
Copy link
Member
1st1 commented Oct 20, 2020

@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 = {
Copy link
Member

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.

Copy link
Member
@1st1 1st1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typedef struct {
unaryfunc am_await;
unaryfunc am_aiter;
unaryfunc am_anext;
sendfunc am_send;
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor

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

/* 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) {
Copy link
Member

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.

Suggested change
if (type->tp_flags & Py_TPFLAGS_HAVE_SEND) {
if (type->tp_flags & Py_TPFLAGS_HAVE_AM_SEND) {

static PyObject *
FutureIter_iternext(futureiterobject *it)
static PySendResult
FutureIter_am_send(futureiterobject *it, PyObject *Py_UNUSED(arg), PyObject **result)
Copy link
Member

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

@1st1
Copy link
Member
1st1 commented Oct 21, 2020

@serhiy-storchaka could you please review again?

@1st1
Copy link
Member
1st1 commented Oct 23, 2020

@pablogsal Pablo maybe you can take a quick look at this?

@pablogsal
Copy link
Member

@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.

@1st1
Copy link
Member
1st1 commented Oct 30, 2020

@scoder @serhiy-storchaka Guys, could you please take another look? I plan to merge this soon.

Copy link
Contributor
@asvetlov asvetlov left a 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

@1st1
Copy link
Member
1st1 commented Nov 10, 2020

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 1st1 merged commit 1e996c3 into python:master Nov 10, 2020
@bedevere-bot
Copy link

@1st1: Please replace # with GH- in the commit message next time. Thanks!

@vladima vladima deleted the send_slot branch November 10, 2020 20:16
asvetlov added a commit to asvetlov/cpython that referenced this pull request Nov 11, 2020
asvetlov added a commit that referenced this pull request Nov 11, 2020
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants
0