bpo-43751: Fix anext() bug where it erroneously returned None #25238
Conversation
|
There are a bunch of problems in the CI. The main one is: and there is also some trailing whitespace. Could you check these? |
|
This should probably have been a draft PR. I wrote a test for the erroneous None returns, then fixed it. And then I applied the same test to async iterables that are not generators, and that is still failing. |
|
This should be ready for review now. |
|
Side comment on the PR title: please don't use "should no longer". Say something like "Fix anext() bug where it erroneously returned None". |
Fix inconsistent variable names in comment Co-authored-by: Joshua Bronson <jabronson@gmail.com>
|
I'm requesting that someone else reviews this instead of me. @pablogsal? @1st1? |
| return NULL; | ||
| } | ||
| unaryfunc getter = Py_TYPE(awaitable)->tp_as_async->am_await; | ||
| PyObject *new_awaitable = getter(awaitable); |
There was a problem hiding this comment.
This can raise, you need to check for errors and propagate accordingly
There was a problem hiding this comment.
Also, can you add a test that checks this code path?
There was a problem hiding this comment.
I added the check, but I couldn't figure out how to make a test to check the code path, since awaitable is always a coroutine, so getter points to this in genobject.c:
static PyObject *
coro_await(PyCoroObject *coro)
{
PyCoroWrapper *cw = PyObject_GC_New(PyCoroWrapper, &_PyCoroWrapper_Type);
if (cw == NULL) {
return NULL;
}
Py_INCREF(coro);
cw->cw_coroutine = coro;
_PyObject_GC_TRACK(cw);
return (PyObject *)cw;
}There was a problem hiding this comment.
You need to add a test to the _testcapi module, but this is going to be a bit too much for just this, so is fine if we don't add such a test.
There was a problem hiding this comment.
Wait, isn't this something you can get with a custom __await__ in a class?
There was a problem hiding this comment.
_PyCoro_GetAwaitableIter calls am_await and ensures it's iterable; this if block is only for coroutines.
There was a problem hiding this comment.
test_anext_bad_await covers __await__ methods that raise
…use awaitable is always a coroutine in that case.
| } | ||
| Py_SETREF(awaitable, new_awaitable); | ||
| if (Py_TYPE(awaitable)->tp_iternext == NULL) { | ||
| PyErr_SetString(PyExc_TypeError, |
There was a problem hiding this comment.
I think this is leaking awaitable.
| return NULL; | ||
| } | ||
| } | ||
| PyObject *result = (*Py_TYPE(awaitable)->tp_iternext)(awaitable); |
There was a problem hiding this comment.
You need to decrement awaitable once you are done with it
|
|
|
Thanks for the PR @sweeneyde |
https://bugs.python.org/issue43751