Port fixes for unchaining exceptions in contextmanager from cpython

[remleduff]

Cherry-pick 647438fa5a7b9a5fb3fd1af57541b52646c7a013,
bpo-29692: contextlib.contextmanager may incorrectly unchain RuntimeError

Fixes #203

[njsmith]

Hey, thanks for putting this together! Two small comments:

[njsmith]

The problem with writing tests this way is that if no exception is raised, then everything looks good... so it's generally better practice to do:

with pytest.raises(RuntimeError) as excinfo:
    ...
assert excinfo.value.args[0] == "..."
...

which will actually fail if it doesn't see a RuntimeError. OTOH if this is copied directly from the upstream fix and you'd rather keep it close to upstream there's a reasonable argument for that.

[remleduff]

Went ahead and fixed it up as you suggested. I was just copy-pasting from upstream, but I think it's nicer this way.

[njsmith]

Unfortunately we still support py35, so we can't use native generators like this in the test suite (this is why appveyor and travis are failing) – it needs to be like

@async_generator
async def test_issue29692():
    try:
        await yield_()
    except ...

[remleduff]

I was just digging into this. Unfortunately, it seems that when I try it this way (actually using both async_generator and acontextmanager decorators), the actual fix doesn't work anymore.

[njsmith]

Uh oh. Should I look more closely? async_generator is a pretty solid emulation of the built-in async generators, but it's had subtle bugs before....

One thing that just popped out at me looking again though: we surely want to replace all the StopIterations in this patch with StopAsyncIterations

[remleduff]

That was it, thanks! Weird that it worked with the wrong exception using native generators.

[njsmith]

.....okay, I just re-read PEP 479 and this is more complicated than I realized. I'm actually not sure that what I said above about replacing StopIteration with StopAsyncIteration is correct, and actually it's quite likely that async_generator doesn't handle this correctly. Need to do some experiments to see exactly what the python 3.6 native async generator behavior actually is...

[remleduff]

OK, current version has passing tests for what it's worth, but I'll sign off for now

[codecov]

Codecov Report

Merging #228 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
+ Coverage   99.06%   99.09%   +0.02%     
==========================================
  Files          63       63              
  Lines        8420     8466      +46     
  Branches      609      610       +1     
==========================================
+ Hits         8341     8389      +48     
+ Misses         62       61       -1     
+ Partials       17       16       -1

Impacted Files	Coverage Δ
trio/tests/test_util.py	100% <100%> (ø)	⬆️
trio/_util.py	90% <100%> (+2.35%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6311069...a002811. Read the comment docs.

[njsmith]

Here's my current understanding of this.

The problem being fixed is: suppose we have some code like:

try:
    with my_context_manager():
        raise StopIteration
except StopIteration:
    print("caught it!")

this is totally fine and normal, and should print caught it! (unless the context manager specifically catches this exception, but let's assume it doesn't). Now let's assume my_context_manager() is implemented as a contextlib.contextmanager wrapper around a generator. Then the way this code should work is that StopIteration will be thrown into the generator, which lets it escape... and then we have a problem! PEP 479 made it so that if a generator raises StopIteration, this gets automatically converted into a RuntimeError. In general this is a good thing because it lets us tell the difference between a generator that raises StopIteration, and one that exits normally (which causes the interpreter to automatically raise StopIteration). But it makes life complicated for contextlib.contextmanager, because if it lets this RuntimeError escape, then it'll give the wrong answer on our code above.

So the solution is that the contextmanager code needs to detect when we've got a RuntimeError that was wrapped around a StopIteration, and unwrap it again. And that's what the version I originally copied this code from tried to do. However, it was a bit over-eager in how it did this: it unwrapped any RuntimeError that it saw that had a __cause__ attribute, not just RuntimeErrors that were automatically wrapped around a StopIteration. So the idea of the patch we're trying to adapt, is that it makes this check more careful. The two tests are: (a) now if we get a RuntimeError that's wrapped around something that's not StopIteration, then the RuntimeError should be preserved, and (b) if we do get a RuntimeError that's wrapped around a StopIteration, then it should continue to be correctly unwrapped.

Phew, ok, now we understand what's going on upstream with regular synchronous contextmanager. How does this translate to our async acontextmanager? This is complicated because internally async generators use both StopIteration (to signal the end of each call to anext/asend/athrow) and also StopAsyncIteration (to signal the end of the overall iteration).

I don't think this is documented anywhere, but based on some experimentation, it looks like the way PEP 479 was applied to the native async generators in 3.6 is: if they raise either StopIteration or StopAsyncIteration, then that gets wrapped into a RuntimeError.

So this suggests that acontextmanager's unwrapping code needs to check for both of these special exception types, and indeed, that's exactly what the upstream asynccontextmanager (only available in 3.7-dev builds) does.

Then there's an additional wrinkle: it looks like currently, async_generator async generators correctly wrap StopIteration in RuntimeError, but they don't correctly wrap StopAsyncIteration in RuntimeError – they just let StopAsyncIteration pass through unchanged.

So:

We should make the unwrapping logic check for both StopIteration and StopAsyncIteration, like the upstream asynccontextmanager does.

We should probably test throwing both StopIteration and StopAsyncIteration through an async context manager (like the last test in here does currently).

From comments above, it sounds like making that last test use StopIteration doesn't actually work? If so then I'm confused – it seems like it should work to me!

And currently that last test isn't actually testing the unwrapping logic, because it's using StopAsyncIteration + an async_generator-based generator, so the exception is never getting wrapped in the first place. I'm looking at making async_generator properly implement PEP 479, but I'm currently hitting some weird error that I don't understand.

Arguably this is not a big deal though because acontextmanager is not a public API, and all of our internal users use it with async_generator-based generators, so as long as those two working together give the right result it doesn't really matter why.

[njsmith]

Also, sorry about this -- I really underestimated how complicated this would be when I put the good-for-new-contributors tag on #203!

[njsmith]

I'm looking at making async_generator properly implement PEP 479, but I'm currently hitting some weird error that I don't understand.

I just sorted this out and released async_generator 1.8, which now properly implements PEP 479. Going to try hitting the rebuild buttons for this PR now and see if it changes anything...

[njsmith]

Well, Travis is doing something weird, hopefully it will wake up and run the tests eventually, but in the mean time this worked on appveyor, so it's probably OK :-). Hopefully that means that we just need a few more small changes:

[njsmith]

Well, Travis is doing something weird, hopefully it will wake up and run the tests eventually, but in the mean time this worked on appveyor, so it's probably OK :-). Hopefully that means that we just need a few more small changes:

[njsmith]

Should check for StopIteration as well. I guess if issubclass(type, (StopIteration, StopAsyncIteration)) and exc.__cause__ is value: might be the most idiomatic way to write it

[remleduff]

I copied the code from the 3.7 version you linked and used "isinstance(value, (StopIteration, StopAsyncIteration)" which appeared to be equivalent to your suggestion.

[njsmith]

This last line (raise RuntimeError(...)) is redundant -- there's already another check for the same thing up above. Should be deleted.

[njsmith]

...And we need to a copy of this test, except with raise StopIteration(...) replacing raise StopAsyncIteration

[remleduff]

Thanks for the intense commentary, very interesting. I think I follow. ;)

I've updated everything with your comments and grabbed the newest async_generator, and most things look better, but still running into one issue.

I added some tests gated on version that use native async generators and they all pass, but using async_generator passes for StopAsyncIteration and fails for StopIteration.

I've dug in a bit, but hope you see something obvious that I'm missing.

Pushed my updates for now.

[njsmith]

Thanks for the intense commentary, very interesting. I think I follow. ;)

Feel free to ask for clarifications if I get too telegraphic! Often when I write a big chunk of text like that I'm partly writing it as a way to think things through for myself, so I sometimes do a bad job of making it interpretable to other people – but part of the goal is to explain to others and to leave a record for when we come back to an issue later. And both of these work much better if someone pokes me when I'm being unclear :-)

[njsmith]

Unfortunately 3.5 blows up if you even have syntax like this in your file, it doesn't matter whether you try executing it or not. So the only way I know to write a test like this is to use exec:

code = textwrap.dedent("""
    @acontextmanager
    async def test_issue29692_2():
        ...
""")
ns = {"acontextmanager": acontextmanager}
exec(code, ns)
test_issue29692_2 = ns["test_issue29692_2"]

(and then you still need a version check to make sure you only do this on 3.6+, or alternatively try it always and then catch the SyntaxError if it doesn't work)

It might make sense to consolidate with the previous test... like make a version dependent list of test context managers, and then do a for loop over it? Or not, sometimes it's simpler just to have a bit of copy-paste in test code :-)

[remleduff]

Yeah, was just discovering that myself. I hadn't actually installed Python3.5 until about 15 minutes ago and didn't realize what I was doing didn't work until Travis let me know.

[njsmith]

I hadn't actually installed Python3.5 until about 15 minutes ago

That's very sensible of you really... I'm surprised at how quickly the py35 support is becoming an annoying limitation :-(. I'm really reluctant to drop pypy support though, and they don't even have an ETA for starting to do 3.6 yet :-(.

[remleduff]

Got the test stuff fixed up on my computer for both versions of python.

If I'm understanding you correctly, we're ok with some differences in async_generator for StopIteration, so I just fleshed that out enough to know what the result is at the moment.

Probably not a huge deal until Python3.5 support isn't needed anymore, hopefully.

[njsmith]

If I'm understanding you correctly, we're ok with some differences in async_generator for StopIteration

Yep!

[njsmith]

I think this marks the whole file as skipped on 3.5? That doesn't seem like what we want. IIRC the syntax is

# skip a single test
@pytest.mark.skipif(check, reason="whatever")
def test_something():
   ...

# skip multiple tests with the same reason
need_native_async_generators = pytest.mark.skipif(...)

@need_native_async_generators
def test_something():
   ...

@need_native_async_generators
def test_something_else():
   ...

[njsmith]

OKAY, I figured out the problem with StopIteration and async_generator!

Here it is: when a StopIteration gets thrown into an async_generator, it actually ends up inside the magic await yield_() function. But Python doesn't know that it's supposed to be a magic function that acts like yield; it just thinks its some random async function. And then it tries to raise StopIteration, and Python's like uh, no, haven't you heard of PEP 479? Async functions can't raise StopIteration. So it immediately wraps it into a RuntimeError. Which ... is a little weird and annoying, but there's nothing we can do about it, and it doesn't stop us from unwrapped it later. Except... the way this particular test is written, where it wraps everything in a RuntimeError, means that we end up wrapping our StopIteration in a RuntimeError twice. We get RuntimeError → RuntimeError → StopIteration.

So there is something of a bug here, but it's intrinsic to how async_generator works, and we can't fix that part.

But! We can still test the main thing we care about, which is that a StopIteration is able to pass through a context manager like

@acontextmanager
@async_generator
async def simpler_context_manager():
    await yield_()

And this should work, though slightly differently than if it were a native generator. With a native generator, the yield would raise StopIteration, which would then get wrapped in a RuntimeError when it tried to leave the function. With async_generator, the await yield_() directly wraps the StopIteration into a RuntimeError, and then that propagates through the function and out. But either way we can catch it and unwrap it in @acontextmanager.

[njsmith]

See #229 for an issue to record the weird edge case that this creates in trio proper...

[kyrias]

Canceled and restarted the job since Travis was having some maintenance hopefully it gets scheduled now?

[kyrias]

Seems they have some problems with non container builds actually: https://www.traviscistatus.com

Backlogs keep building without being processed. Build slaves fell over maybe.

[njsmith]

Thanks!

It looks like I missed doing this on your last PR – our policy is that anyone who submits at least 1 merged PR automatically gets an invitation to join the github organization if they want it. So, check your inbox! No pressure – it's totally up to you whether you want to accept, and if you do then you can participate as much or as little as you like. Basically this is our way of saying hey, welcome, we'd love it if you want to stick around :-).