classification
Title: PEP 479: Change StopIteration handling inside generators
Type: enhancement Stage: resolved
Components: Interpreter Core, Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: yselivanov Nosy List: Rosuav, Yury.Selivanov, belopolsky, berker.peksag, ethan.furman, gvanrossum, haypo, john.black.3k, ncoghlan, neil.g, python-dev, r.david.murray, rhettinger, schlamar, scoder, serhiy.storchaka, yselivanov
Priority: release blocker Keywords: patch

Created on 2014-11-20 10:43 by Rosuav, last changed 2015-10-07 01:43 by john.black.3k. This issue is now closed.

Files
File name Uploaded Description Edit
pep0479.patch Rosuav, 2014-11-20 10:43 POC patch for PEP 479 review
itertoolsdoc.diff gvanrossum, 2014-11-27 20:50 review
pep0479.patch Rosuav, 2015-01-08 10:38 Replacement POC patch review
stopiter.py Rosuav, 2015-01-08 10:41 Simple demo of new behaviour
pep0479.patch Rosuav, 2015-01-08 14:50 Real patch, creates __future__ directive and all review
test_pep479.py Rosuav, 2015-04-16 23:31
pep0479.patch yselivanov, 2015-05-07 21:25 review
pep0479.patch yselivanov, 2015-05-08 00:33 review
pep0479.patch yselivanov, 2015-05-08 15:33 review
Messages (70)
msg231422 - (view) Author: Chris Angelico (Rosuav) * Date: 2014-11-20 10:43
See PEP for full details. Attached is POC patch: behaviour is altered globally (rather than respecting a __future__ directive), and minimal changes are made elsewhere to make the test suite mostly pass (test_generators does not - it'll need more comprehensive edits). Note that PEP 8 is deliberately not followed here; some of the edits ought to involve indenting slabs of code, but instead, "half-level" indentation is used, to keep the patch visibly minimal.
msg231447 - (view) Author: Roundup Robot (python-dev) Date: 2014-11-20 19:30
New changeset dd19add74b21 by Guido van Rossum in branch 'default':
PEP 479: Add link to issue 22906.
https://hg.python.org/peps/rev/dd19add74b21
msg231478 - (view) Author: Marc Schlaich (schlamar) * Date: 2014-11-21 17:17
AFAIS this would break all existing code for yield-based coroutine schedulers (Tornado, Twisted, Trollius, monocle, ...) when a coroutine is exited with `raise StopIteration` in client code. 

And this seems like a lot, a quick GitHub code search gives various examples, e.g.

- https://github.com/circus-tent/circus-web/blob/3b76c83d1eb984a78ed69a9abcb13054cde78f89/circusweb/circushttpd.py#L170
- https://github.com/kzahel/falcon-api/blob/5c62454db971bc99c52694cf2ce0613fb1504d80/python/falcon_api/session.py#L159
- https://github.com/peterhajas/Genesis/blob/2f1a03c38934b892ee6bb04e07e867bf463b7ae5/servers/genesis/clients.py#L24
msg231481 - (view) Author: Chris Angelico (Rosuav) * Date: 2014-11-21 17:24
Marc, are those all cases where the "raise StopIteration" is actually inside a generator? If so, it can be trivially replaced with "return". Yes, it'll break that way of spelling it, but it's a completely mechanical transformation, and making the change won't break your code for previous versions of Python. Personally, I would recommend using "return" there anyway, regardless of this proposal.
msg231482 - (view) Author: Marc Schlaich (schlamar) * Date: 2014-11-21 17:35
Yes, all yield-based coroutines are generators. I know that there is a backward compatible upgrade path, but this might have a huge impact on existing code.

Interestingly, I didn't know before researching this PEP that you can actually use `return` without arguments in generators before Python 3.3 (even in 2.3) and I have worked a lot with coroutines/generators.

So I'm not even against this proposal and using `return` instead of `raise StopIteration` seems the right way to exit a generator/coroutine, but there could be lots of affected users...
msg231483 - (view) Author: Chris Angelico (Rosuav) * Date: 2014-11-21 17:44
Yep, the question is whether any of the "raise StopIteration" lines are actually non-local flow control. If they're local, then it's easy: mechanical replacement with "return" and it becomes compatible with all versions (unless it has a value attached to it, as "return x" doesn't work in Py2). But if they're non-local, some refactoring will need to be done.

In any case, my line of argument is: A generator function is not an iterator's __next__ method, ergo iterator protocol does not apply. Use of StopIteration is a hack that happens to work because of how generator functions are implemented (a thin wrapper around an iterator), but it's not part of the *concept* of a generator function.
msg231485 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-11-21 18:11
If all examples were just using "raise StopIteration" instead of "return"
in a generator I would be very happy. Such code can be trivially fixed by
using "return" and the __future__ import will help the eventual transition.

It's sad that apparently this use of return hasn't been better advertised
-- it has existed since generators were first introduced.

On Fri, Nov 21, 2014 at 9:44 AM, Chris Angelico <report@bugs.python.org>
wrote:

>
> Chris Angelico added the comment:
>
> Yep, the question is whether any of the "raise StopIteration" lines are
> actually non-local flow control. If they're local, then it's easy:
> mechanical replacement with "return" and it becomes compatible with all
> versions (unless it has a value attached to it, as "return x" doesn't work
> in Py2). But if they're non-local, some refactoring will need to be done.
>
> In any case, my line of argument is: A generator function is not an
> iterator's __next__ method, ergo iterator protocol does not apply. Use of
> StopIteration is a hack that happens to work because of how generator
> functions are implemented (a thin wrapper around an iterator), but it's not
> part of the *concept* of a generator function.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22906>
> _______________________________________
>
msg231486 - (view) Author: Chris Angelico (Rosuav) * Date: 2014-11-21 18:13
Sadly, I don't know of a way to check if that's the case, other than by manually going through and eyeballing the code - if there's "raise StopIteration", see if there's also "yield" in the same function. The three cited examples are (I checked those straight away), but frankly, I am not volunteering to go manually through all of github in search of examples :|
msg231487 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-11-21 18:26
I suggest to apply right now those changes which are compatible with current behavior and don't make code more cumbersome. E.g.

-        while True:
-            yield next(line_pair_iterator)
+        yield from line_pair_iterator

and

-        raise StopIteration
+        return

To me these changes make code even better and are worth to be applied even if PEP 479 will be rejected.
msg231675 - (view) Author: Chris Angelico (Rosuav) * Date: 2014-11-25 18:02
Known issues with the current patch, if anyone feels like playing with this who better knows the code:

1) Needs a __future__ directive to control behaviour
2) test_generators needs to be heavily reworked
3) The test of what exception was thrown needs to also handle StopIteration subclasses
4) Check for refleaks and/or over-freeing
5) Properly provide a traceback for the original StopIteration (not always happening)

Any others?
msg231779 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-11-27 20:50
Here's a doc patch for itertools.
msg231826 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-11-28 19:49
FYI:  I applied these two changes right after Guido pronounced on PEP 479:

https://mail.python.org/pipermail/python-checkins/2014-November/133252.html

https://mail.python.org/pipermail/python-checkins/2014-November/133253.html

Also, I'm submitting a patch to fix the code in Django that will be broken by PEP 479.
msg231828 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-11-28 20:50
> FYI:  I applied these two changes right after Guido pronounced on PEP 479:

Extract of emails:

changeset:   93542:9eb0d0eb0992
parent:      93540:23f8a511050a
user:        Raymond Hettinger <python at rcn.com>
date:        Sat Nov 22 21:56:23 2014 -0800

PEP 479:  Don't let StopIteration bubble out of calls to next() inside a generator.


changeset:   93543:e8b3083bb148
user:        Raymond Hettinger <python at rcn.com>
date:        Sat Nov 22 22:14:41 2014 -0800

PEP 479:  Use the return-keyword instead of raising StopIteration inside a generators.
msg231852 - (view) Author: Stefan Behnel (scoder) * Date: 2014-11-29 08:17
Regarding the patch below, isn't most of this redundant? ISTM that simply calling PyErr_SetString(...) should do all of this, including the exception chaining.


diff -r 23ab1197df0b Objects/genobject.c
--- a/Objects/genobject.c	Wed Nov 19 13:21:40 2014 +0200
+++ b/Objects/genobject.c	Thu Nov 20 16:47:59 2014 +1100
@@ -130,6 +130,23 @@
         }
         Py_CLEAR(result);
     }
+    else if (!result)
+    {
+        if (PyErr_ExceptionMatches(PyExc_StopIteration))
+        {
+            PyObject *exc, *val, *val2, *tb;
+            PyErr_Fetch(&exc, &val, &tb);
+            PyErr_NormalizeException(&exc, &val, &tb);
+            Py_DECREF(exc);
+            Py_XDECREF(tb);
+            PyErr_SetString(PyExc_RuntimeError,
+                "generator raised StopIteration");
+            PyErr_Fetch(&exc, &val2, &tb);
+            PyErr_NormalizeException(&exc, &val2, &tb);
+            PyException_SetContext(val2, val);
+            PyErr_Restore(exc, val2, tb);
+        }
+    }
 
     if (!result || f->f_stacktop == NULL) {
         /* generator can't be rerun, so release the frame */
msg231853 - (view) Author: Chris Angelico (Rosuav) * Date: 2014-11-29 08:38
Stefan, I'm not sure - I don't know the details of the C API here. But I tried commenting out everything but that one line, and while it does result in RuntimeError, it doesn't do the exception chaining. Currently, I believe the exception isn't being caught at all; but I don't know what it would take to do the full catching properly. The current patch doesn't always have a traceback on the original StopIteration, either, so it's definitely not quite right yet.
msg231855 - (view) Author: Stefan Behnel (scoder) * Date: 2014-11-29 08:59
Ah, right - chaining only happens automatically when the exception has already been caught and moved to sys.exc_info.

There's a _PyErr_ChainExceptions(), though, which does it for you. You should be able to say

    PyErr_Fetch(&x,&y,&z)
    PyErr_SetString()
    _PyErr_ChainExceptions(x,y,z)

(does pretty much what your code does as well)
msg231856 - (view) Author: Chris Angelico (Rosuav) * Date: 2014-11-29 09:02
Yeah, I saw that. Since that function begins with an underscore, I thought it best to replicate its behaviour rather than to call it. Either way ought to work though.
msg231857 - (view) Author: Stefan Behnel (scoder) * Date: 2014-11-29 09:10
Public underscore C functions are there for exactly the purpose of not duplicating functionality across *internal* core runtime code, without making them an official part of the C-API for *external* code. (I understand that it's a POC patch, though, so whoever applies that change eventually will rework it anyway.)
msg231858 - (view) Author: Stefan Behnel (scoder) * Date: 2014-11-29 09:27
FYI, here's the set of tests that I've written for my implementation in Cython:

https://github.com/cython/cython/blob/b4383a540a790a5553f19438b3fc22b11858d665/tests/run/generators_pep479.pyx
msg233620 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-01-08 03:33
As Stefan noted, the leading underscore on "_PyErr_ChainExceptions" is just a warning to third party users that we don't yet offer any API stability guarantees for it. Using it in CPython itself is entirely appropriate - that's the whole reason for exposing it to the linker at all.

It would be good to have a public API for that functionality though, so I filed issue 23188 to suggest making it public.

The main thing missing from the patch at this point is the __future__ import. To learn what's involved in that, one of the useful tricks is to look at the annotated __future__ module: https://hg.python.org/cpython/annotate/bbf16fd024df/Lib/__future__.py

The commits adding new flags there highlight the various places that tend to be affected when a new __future__ import is added. For this particular case, the closest comparison is likely with the "absolute import" feature. One interesting aspect of this particular flag though is that it doesn't really affect compilation at all, aside from setting the flag on the code objects to indicate the future statement was in effect. The main impact will be at runtime, where it will make the exception replacement in Chris's PoC patch conditional on the presence of the flag on the code object.

Chris, will you have time in the near future to rework the patch to add the __future__ support? It would be good to get this into 3.5a1 to give it the longest possible settling in period.
msg233621 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-01-08 03:34
Heh, I guess that should have been "The main thing missing other than docs and tests is ..." :)
msg233622 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-01-08 03:46
I can have a poke at the __future__ import tonight, but my main concern is memory management - I'm not sufficiently familiar with the exception handling calls to be sure that I'm neither leaking nor over-freeing anything. There's also a secondary concern that the tracebacks aren't quite right at the moment, possibly caused by a muck-up in the exception chaining.

>>> def f(): raise StopIteration
>>> def g(): yield f()
>>> next(g())
StopIteration

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: generator raised StopIteration

There's no indication of which function/line caused the exception, which would be _extremely_ helpful. If someone else can look into that at some point, I'd appreciate the assistance.
msg233629 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-01-08 08:47
Looking more closely at the patch:

* for the missing tracebacks, you're probably hitting the note in https://docs.python.org/3/c-api/exceptions.html#c.PyErr_NormalizeException and need an explicit call to PyErr_SetTraceback (My recollection is that I made this same mistake when working on the codec chaining patch, so we may want to revisit the comment before PyErr_NormalizeException that suggests making setting the traceback attribute on the normalized value implicit)

* to trigger the implicit chaining in PyErr_String, try calling PyErr_Restore on the normalized exception before calling PyErr_SetString, rather than just dropping the references. That should let you drop the subsequent explicit PyErr_SetContext call.
msg233640 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-01-08 10:38
PyErr_Restore doesn't seem to trigger exception chaining. But thanks for the tip about explicitly setting the traceback; not sure how I missed that, but now the StopIteration traceback is visible.

Minor point: The previous patch was setting the __context__ of the RuntimeError, whereas it ought to have been setting __cause__. Have corrected that.

So here, before I move further forward, is a new POC patch; I've removed the patches that rhettinger applied already, and fixed up tracebacks. So now it's a better-behaved POC, at least.
msg233645 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-01-08 11:19
Nick, any particular reason for pointing to https://hg.python.org/cpython/annotate/bbf16fd024df/Lib/__future__.py rather than https://hg.python.org/cpython/annotate/tip/Lib/__future__.py ? I'm looking at both, anyhow.
msg233665 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-01-08 14:50
Okay! I think I have something here. DEFINITELY needs more eyeballs, but all tests pass, including a new one that tests StopIteration leakage both with and without the __future__ directive. Some docs changes have been made (I grepped for 'stopiteration' and 'generator' case insensitively, and changed anything that caught my eye). It'd be nice if the entire test suite and standard library could be guaranteed to work in a post-3.7 world, but I don't know of an easy way to do that, short of peppering the code with unnecessary __future__ directives.
msg237223 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-03-05 00:20
Looking for code reviewers!
msg237247 - (view) Author: Neil Girdhar (neil.g) * Date: 2015-03-05 08:24
FWIW I looked at the changes.  Does it make sense to run tests before there are actual tests in lib/Test?  I'll happily run all tests when some new ones are added.
msg237277 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-03-05 17:25
Thanks, Neil, for catching that.

I did run the entire test suite with the patch, and nothing new broke, so it would seem the patch is at least benign.  :)
msg237278 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2015-03-05 17:25
Oh, and my tests ran on Ubuntu 13.04 (GNU/Linux 3.8.0-22-generic x86_64).
msg238449 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-03-18 14:49
Bumped to release blocker so we make sure to look at this at the PyCon sprints, if not before. (I'm assuming Larry's not taking release blocker literally for the 3.5 alphas)

As Neil notes, this patch needs some explicit tests for the new behaviour when the future flag is set.

From a docs point of view, the changes to the language reference itself currently seem surprisingly small, but I haven't looked through in detail to see where other updates might be needed, or how the already modified section could be better updated to account for the impact of the __future__ directive (the current update there doesn't mention that the behaviour is in the process of changing)
msg241255 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-04-16 18:29
Did anyone look at this yet? The ultimate deadline would be May 24 (beta 1). But making alpha 4 (April 19) would be better.
msg241290 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-04-16 23:31
Simple test case for the future directive. Needs expansion.
msg241999 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-04-25 06:58
Unfortunately we didn't find the time to review the currently flagged release blockers at the sprints :(

Chris, for the mechanics of testing the future flag, it's potentially worth looking at the 2.7 branch, as that should have tests for the print_function and unicode_literals flags.
msg242115 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-04-27 13:24
Had a peek at the 2.7 branch in the web (https://hg.python.org/cpython/file/4234b0dd2a54/Lib/test) and all the tests appear to be testing the behaviour *with* the future directive, not bothering to test the old behaviour. It makes sense - that way, when the future directive becomes permanent, there's no suddenly-failing test - can someone confirm that that's the intention?

The current test simply triggers a StopIteration and verifies that RuntimeError comes up off it, without testing the current behaviour, nor testing any of the aspects that haven't changed. I'm basically assuming that generators themselves are adequately tested elsewhere, such that a bug in the PEP 479 code that breaks generators in any other way should be caught by a test failure someplace else. Can anyone think of other aspects of PEP 479 that need to be tested?
msg242630 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-05-06 00:18
Would it be possible to push the first part of the implementation (without __future__) just to unblock the implementation of the PEP 492 (issue #24017: async/await)?

Later push the second part for __future__.
msg242632 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-05-06 00:27
Stinner, not sure what you mean by first part / second part. Is there a way for me to withdraw the first two versions of the patch and just keep #37646?
msg242633 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-05-06 00:27
Well that would break a lot of code...
On May 5, 2015 5:18 PM, "STINNER Victor" <report@bugs.python.org> wrote:

>
> STINNER Victor added the comment:
>
> Would it be possible to push the first part of the implementation (without
> __future__) just to unblock the implementation of the PEP 492 (issue
> #24017: async/await)?
>
> Later push the second part for __future__.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22906>
> _______________________________________
>
msg242698 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-06 18:32
Hi Chris! Can I somehow help with the patch?
msg242709 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-05-07 02:45
You sure can! Take it, deploy it, run the test suite, and then start writing real code that uses it. When you find a problem, that's what needs help! :)
msg242732 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-07 20:16
> You sure can! Take it, deploy it, run the test suite, and then start writing real code that uses it. When you find a problem, that's what needs help! :)

Thank you for this generic answer, Chris.

The reason I was asking is because issue #24017 depends on this one (also release blocker).  And I was genuinely wondering if I can help (somehow) advancing your patch to be committed asap.  Anyways, do you have any estimate when you finalize it?
msg242733 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-05-07 20:18
I think you could help by (a) reviewing what's there, and (b) helping with
the implementation of __future__.

On Thu, May 7, 2015 at 1:16 PM, Yury Selivanov <report@bugs.python.org>
wrote:

>
> Yury Selivanov added the comment:
>
> > You sure can! Take it, deploy it, run the test suite, and then start
> writing real code that uses it. When you find a problem, that's what needs
> help! :)
>
> Thank you for this generic answer, Chris.
>
> The reason I was asking is because issue #24017 depends on this one (also
> release blocker).  And I was genuinely wondering if I can help (somehow)
> advancing your patch to be committed asap.  Anyways, do you have any
> estimate when you finalize it?
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22906>
> _______________________________________
>
msg242734 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-05-07 20:24
Or, if it's perfect (or good enough :-), just check it in.

On Thu, May 7, 2015 at 1:18 PM, Guido van Rossum <report@bugs.python.org>
wrote:

>
> Guido van Rossum added the comment:
>
> I think you could help by (a) reviewing what's there, and (b) helping with
> the implementation of __future__.
>
> On Thu, May 7, 2015 at 1:16 PM, Yury Selivanov <report@bugs.python.org>
> wrote:
>
> >
> > Yury Selivanov added the comment:
> >
> > > You sure can! Take it, deploy it, run the test suite, and then start
> > writing real code that uses it. When you find a problem, that's what
> needs
> > help! :)
> >
> > Thank you for this generic answer, Chris.
> >
> > The reason I was asking is because issue #24017 depends on this one (also
> > release blocker).  And I was genuinely wondering if I can help (somehow)
> > advancing your patch to be committed asap.  Anyways, do you have any
> > estimate when you finalize it?
> >
> > ----------
> >
> > _______________________________________
> > Python tracker <report@bugs.python.org>
> > <http://bugs.python.org/issue22906>
> > _______________________________________
> >
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22906>
> _______________________________________
>
msg242736 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-07 21:25
Hi,

Please find attached an updated patch.

Summary of changes:

1. Most of feedback from Nick Coghlan and Serhiy Storchaka is applied;

2. Changes in difflib.py were reverted (unless we add the __future__ import there right now there is no need to fix it);

3. Chris' test is integrated to the patch.

All in all I think it's in a good shape now, but I'd appreciate if someone looks at genobject.c changes one more time.
msg242740 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-07 23:13
Yury's patch mostly looks good to me, except:

* the check in contextlib should be against __cause__ rather than
__context__, and there should be a new test for this code handling path
* there should be a new test for the __future__ flag itself (independently
of the contextlib tests)
msg242741 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-08 00:33
> Yury's patch mostly looks good to me, except:

Thanks!

> * the check in contextlib should be against __cause__ rather than
__context__, and there should be a new test for this code handling path

Done.  I've also added one test for correct handling of StopIteration without PEP 479.

> * there should be a new test for the __future__ flag itself (independently
of the contextlib tests)

Forgot to attach it to the first patch!

Nick, please take a look at the new patch (attached).
msg242743 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-05-08 02:16
The comment was general because I honestly had no idea what was needed still. All I knew was that the patch seemed to work for me, all tests passing (including the new one). Thanks for uploading the new patch; it compiles happily, and I'm running tests now, although that probably won't prove anything new.
msg242746 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-05-08 05:06
A minor comment about the __future__ changes: 3.5.0a1 should probably be 3.5.0b1.
msg242752 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-08 09:24
A couple of minor comments:

* "self.fail" with an appropriate error message is a clearer way to
indicate an incorrect logic path has been hit in a test case

* the details of the exception chaining don't quite look right, as I
believe "raise X from Y" sets both the cause and the context, with the
suppress_context attribute set to turn off the display of the latter. I
suggest checking that in Python code to confirm ny recollection, and then
adjusting the test and implementation if necessary to match the Python
level equivalent
msg242773 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-08 15:33
Nick, Berker,
Please see the updated patch.
msg242785 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-08 23:13
Latest patch LGTM, although I believe the new chaining behaviour checks
would be clearer with the 3 try/except blocks merged into a single block
where all 3 behaviours are checked in the same except clause, and the else
clause complains that StopIteration was suppressed.

(I don't see a need to request a new review for that particular change,
though)
msg242795 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-05-09 03:46
In Lib/__future__.py:

+generator_stop = _Feature((3, 5, 0, "alpha", 1),

"alpha" needs to be changed to "beta".
msg242814 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-09 15:44
New changeset 36a8d935c322 by Yury Selivanov in branch 'default':
PEP 479: Change StopIteration handling inside generators.
https://hg.python.org/cpython/rev/36a8d935c322
msg242815 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-09 15:45
Thanks Nick and Berker for the reviews!
msg242817 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-05-09 16:07
Thanks everyone for all the help getting this to land! This is going to be a part of my active python3 binary from now on :)
msg242819 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-05-09 16:38
Buildbots are not happy:

[ 63/393] test_contextlib
Fatal Python error: Objects/frameobject.c:429 object at 0x200041abc28 has negative ref count -2604246222170760230

Current thread 0x00000200002c2500 (most recent call first):
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/unittest/case.py", line 579 in run
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/unittest/case.py", line 627 in __call__
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/unittest/suite.py", line 122 in run
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/unittest/suite.py", line 84 in __call__
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/unittest/suite.py", line 122 in run
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/unittest/suite.py", line 84 in __call__
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/unittest/suite.py", line 122 in run
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/unittest/suite.py", line 84 in __call__
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/unittest/runner.py", line 176 in run
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/test/support/__init__.py", line 1775 in _run_suite
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/test/support/__init__.py", line 1809 in run_unittest
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/test/regrtest.py", line 1279 in test_runner
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/test/regrtest.py", line 1280 in runtest_inner
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/test/regrtest.py", line 967 in runtest
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/test/regrtest.py", line 763 in main
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/test/regrtest.py", line 1564 in main_in_temp_cwd
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/test/__main__.py", line 3 in <module>
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/runpy.py", line 85 in _run_code
  File "/mnt/9707/edelsohn/cpython-buildarea/3.x.edelsohn-zlinux-z/build/Lib/runpy.py", line 170 in _run_module_as_main
make: *** [buildbottest] Aborted

http://buildbot.python.org/all/builders/System%20Z%20Linux%203.x/builds/3162/steps/test/logs/stdio

http://buildbot.python.org/all/builders/x86%20Ubuntu%20Shared%203.x/builds/11643/steps/test/logs/stdio
msg242820 - (view) Author: Yury Selivanov (Yury.Selivanov) * Date: 2015-05-09 16:45
Strange, the test suite was running just fine on my machine. I'll take a closer look later today.
msg242821 - (view) Author: Chris Angelico (Rosuav) * Date: 2015-05-09 16:47
Weird. Tests ran fine on my machine too. Interestingly, that number is 0xdbdbdbdbdbdbdbda - does that mean anything? (It's negative 0x2424242424242426, for what that's worth.)
msg242822 - (view) Author: Yury Selivanov (Yury.Selivanov) * Date: 2015-05-09 17:02
I think it crashes in debug mode or something. Somewhere we did too many decrefs.
msg242823 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-09 17:54
New changeset d15c26085591 by Yury Selivanov in branch 'default':
Issue 22906: Add test file.
https://hg.python.org/cpython/rev/d15c26085591
msg242824 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-09 18:09
New changeset 5d8bc813d270 by Yury Selivanov in branch 'default':
Issue 22906: Increment refcount after PyException_SetContext
https://hg.python.org/cpython/rev/5d8bc813d270
msg242825 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-09 18:10
Berker, buildbots should be happy now.
msg242828 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-05-09 18:45
Thanks!
msg242849 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-10 03:40
Since it took me a moment to figure out why the extra incref was needed:

* both PyException_SetCause and PyException_SetContext steal a reference to their second argument
* hence we need the second incref, rather than relying solely on the reference received from PyErr_NormalizeException

This does suggest the new incref is conceptually in the wrong spot - it should be before the call to PyException_SetCause, such that this block of code *always* possesses a valid reference while accessing "val". At the moment, we technically still don't have an active reference when passing "val" to PyException_SetContext.
msg242863 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-10 19:10
New changeset 787cc3d1d3af by Yury Selivanov in branch 'default':
Issue #22906: Do incref before SetCause/SetContext
https://hg.python.org/cpython/rev/787cc3d1d3af
msg243578 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-19 11:18
We missed the deprecation warning part of the PEP (for when the future import is *not* in effect, but the default behaviour will change in 3.7), but rather than reopening this one, I filed a new issue: issue 24237
msg243830 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-22 15:16
New changeset 2771a0ac806b by Yury Selivanov in branch 'default':
Issue 24237: Raise PendingDeprecationWarning per PEP 479
https://hg.python.org/cpython/rev/2771a0ac806b
msg243832 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-22 15:30
New changeset c8a3e49f35e7 by Yury Selivanov in branch 'default':
docs: Mention PEP 479 in whatsnew.
https://hg.python.org/cpython/rev/c8a3e49f35e7
msg252429 - (view) Author: John (john.black.3k) Date: 2015-10-06 20:26
Consider the following generator function similar to the one that started this issue:

    from __future__ import generator_stop

    def myzip(*seqs):
        its = (iter(seq) for seq in seqs)
        while True:
            try:        
                yield tuple(next(it) for it in its)
            except RuntimeError:
                return

    g = myzip('abc', 'xyz')
    print([next(g) for i in range(5)]) # prints: [('a', 'x'), (), (), (), ()]

A print(list(g)) would cause a hang.
msg252443 - (view) Author: John (john.black.3k) Date: 2015-10-07 01:43
Please ignore my previous comment - now I understood what is going on. The `its` generator is a one-shot iterator so is exhausted the first time the while loop is run. Therefore, on subsequent loops, only empty tuples are yielded.

Still learning about generators... :)
History
Date User Action Args
2015-10-07 01:43:49john.black.3ksetmessages: + msg252443
2015-10-06 20:26:25john.black.3ksetnosy: + john.black.3k
messages: + msg252429
2015-05-22 15:30:53python-devsetmessages: + msg243832
2015-05-22 15:16:57python-devsetmessages: + msg243830
2015-05-19 11:18:13ncoghlansetmessages: + msg243578
2015-05-19 11:16:33ncoghlanlinkissue24237 dependencies
2015-05-10 19:10:28python-devsetmessages: + msg242863
2015-05-10 03:40:46ncoghlansetmessages: + msg242849
2015-05-09 18:45:52berker.peksagsetstatus: open -> closed

messages: + msg242828
2015-05-09 18:10:04yselivanovsetmessages: + msg242825
2015-05-09 18:09:16python-devsetmessages: + msg242824
2015-05-09 17:54:04python-devsetmessages: + msg242823
2015-05-09 17:02:32Yury.Selivanovsetmessages: + msg242822
2015-05-09 16:47:53Rosuavsetmessages: + msg242821
2015-05-09 16:45:01Yury.Selivanovsetnosy: + Yury.Selivanov
messages: + msg242820
2015-05-09 16:38:09berker.peksagsetstatus: closed -> open

messages: + msg242819
2015-05-09 16:08:12yselivanovsetstage: commit review -> resolved
2015-05-09 16:07:07Rosuavsetmessages: + msg242817
2015-05-09 15:45:49yselivanovsetstatus: open -> closed
resolution: fixed
messages: + msg242815
2015-05-09 15:44:39python-devsetmessages: + msg242814
2015-05-09 03:46:43berker.peksagsetmessages: + msg242795
stage: test needed -> commit review
2015-05-08 23:13:26ncoghlansetmessages: + msg242785
2015-05-08 15:33:35yselivanovsetfiles: + pep0479.patch

messages: + msg242773
2015-05-08 09:24:27ncoghlansetmessages: + msg242752
2015-05-08 05:06:30berker.peksagsetnosy: + berker.peksag
messages: + msg242746
2015-05-08 02:16:29Rosuavsetmessages: + msg242743
2015-05-08 00:33:19yselivanovsetfiles: + pep0479.patch

messages: + msg242741
2015-05-07 23:13:04ncoghlansetmessages: + msg242740
2015-05-07 21:27:11yselivanovsetassignee: yselivanov
2015-05-07 21:25:05yselivanovsetfiles: + pep0479.patch

messages: + msg242736
2015-05-07 20:24:38gvanrossumsetmessages: + msg242734
2015-05-07 20:18:01gvanrossumsetmessages: + msg242733
2015-05-07 20:16:17yselivanovsetmessages: + msg242732
2015-05-07 02:45:19Rosuavsetmessages: + msg242709
2015-05-06 18:32:06yselivanovsetnosy: + yselivanov
messages: + msg242698
2015-05-06 00:27:42gvanrossumsetmessages: + msg242633
2015-05-06 00:27:06Rosuavsetmessages: + msg242632
2015-05-06 00:18:39hayposetmessages: + msg242630
2015-04-27 13:24:42Rosuavsetmessages: + msg242115
2015-04-25 06:58:03ncoghlansetmessages: + msg241999
2015-04-21 00:53:09yselivanovlinkissue24017 dependencies
2015-04-16 23:31:48Rosuavsetfiles: + test_pep479.py

messages: + msg241290
2015-04-16 18:29:05gvanrossumsetmessages: + msg241255
2015-03-18 14:49:42ncoghlansetmessages: + msg238449
2015-03-18 14:39:27ncoghlansetpriority: normal -> release blocker
2015-03-05 22:05:53ethan.furmansetstage: patch review -> test needed
2015-03-05 17:25:40ethan.furmansetmessages: + msg237278
2015-03-05 17:25:19ethan.furmansetnosy: + ethan.furman
messages: + msg237277
2015-03-05 08:24:59neil.gsetnosy: + neil.g
messages: + msg237247
2015-03-05 00:20:52gvanrossumsettype: enhancement
messages: + msg237223
stage: patch review
2015-01-08 14:50:28Rosuavsetfiles: + pep0479.patch

messages: + msg233665
2015-01-08 11:19:23Rosuavsetmessages: + msg233645
2015-01-08 10:41:15Rosuavsetfiles: + stopiter.py
2015-01-08 10:38:54Rosuavsetfiles: + pep0479.patch

messages: + msg233640
2015-01-08 08:47:20ncoghlansetmessages: + msg233629
2015-01-08 03:46:58Rosuavsetmessages: + msg233622
2015-01-08 03:34:35ncoghlansetmessages: + msg233621
2015-01-08 03:33:19ncoghlansetnosy: + ncoghlan
messages: + msg233620
2014-11-29 09:27:13scodersetmessages: + msg231858
2014-11-29 09:10:23scodersetmessages: + msg231857
2014-11-29 09:02:03Rosuavsetmessages: + msg231856
2014-11-29 08:59:59scodersetmessages: + msg231855
2014-11-29 08:38:37Rosuavsetmessages: + msg231853
2014-11-29 08:17:24scodersetnosy: + scoder
messages: + msg231852
2014-11-28 20:50:32hayposetnosy: + haypo
messages: + msg231828
2014-11-28 19:49:14rhettingersetnosy: + rhettinger
messages: + msg231826
2014-11-28 16:18:31belopolskysetnosy: + belopolsky
2014-11-27 20:50:52gvanrossumsetfiles: + itertoolsdoc.diff

messages: + msg231779
2014-11-25 18:02:40Rosuavsetmessages: + msg231675
2014-11-21 18:26:15serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg231487
2014-11-21 18:17:52r.david.murraysetnosy: + r.david.murray
2014-11-21 18:13:34Rosuavsetmessages: + msg231486
2014-11-21 18:11:13gvanrossumsetmessages: + msg231485
2014-11-21 17:44:46Rosuavsetmessages: + msg231483
2014-11-21 17:35:57schlamarsetmessages: + msg231482
2014-11-21 17:24:16Rosuavsetmessages: + msg231481
2014-11-21 17:17:44schlamarsetnosy: + schlamar
messages: + msg231478
2014-11-20 19:30:53python-devsetnosy: + python-dev
messages: + msg231447
2014-11-20 19:10:34gvanrossumsetnosy: + gvanrossum
2014-11-20 10:43:23Rosuavcreate