Trio102: await inside finally needs shielding and timeout #5
Conversation
|
I'll merge in main to this when you've unbroken it :P |
|
Fixed main, and left you a review on the other PR 😁 |
|
for the shield value, I'll parse constant bools - but do you want to assume True or False for anything else? E.g. |
If it's inside a I'd even be happy to go a step further and say that should be the first statement of the fail/move_on blocks (or passed as an argument to CancelScope), and should not otherwise be assigned to at all. |
I think I'll pass on enforcing it to be the first statement unless you insist, I think current solution is pretty clean. |
…ioScope. Might be a bit overkill, and I'm guessing it'll be more rewrites further down the line
|
Big rewrite, I suspect you might dislike the added class but I I did not see a clean simpler solution. I'm not loving the four separate class variables for tracking state, and them working differently: Will add lineno reference to the finally (and probs fix trio101 at the same time). Could possibly give another reference to the scope if it e.g. has a timeout but no shield. |
|
Oh, is this fine? |
Yep, that's fine - the outer shield protects it from the Cancelled exception that (might have) triggered the |
There was a problem hiding this comment.
I think you're right that there's no clean and simple solution for this - and I think the solution you've got looks pretty good. Some detail-level comments below but at a high level I like it.
For the yield_is_error thing, sure, convert it if you wish, but let's do that in a subsequent PR? I've seen too many new features get delayed by scope-creep 😁
|
This should error, right? with trio.CancelScope(deadline=10, shield=True):
try:
pass
finally:
await foo()such that I should reset the scope whenever entering the finally |
Correct, it should error, because if |
Hmmm, I think that means I need to make scope a stack (which is saved away upon entering finally) and when await'ing search through the stack to see if any of them have shield+timeout. This check certainly isn't getting simpler! |
…at later), raise error on async for and async with, self._scope[s] is now a stack, scopes are reset on entering the finally, fixed coverage issue (and related bug). And added tests for all of the above, as well as separating out py39+ tests into it's own file
|
Fixed all the comments, and I broke out 102 on an impulse. As you said we can revisit that later. The |
Let's just skip referring to the lineno of |
A first draft of trio102. check_for_102 is kind of a mess, with having to inspect a lot of places to find cancel scopes, if they have a timeout specified, and if shielding is set. (It appears there was a shield parameter in the past that now is gone?). It might be a bit defensive and checking the parameter of the trio call is probably silly since they require one.
It also fails on fuzzing site code, since they do have
await's infinally's and this doesn't care if they're related much to trio at all. Maybe add a global check that the file has to import or mention trio?trio102.pyhas a fun case on line 39 (that will fail on python3.8 atm) that idk if it's worth caring about?And it adds duplicates and is generally kinda ugly, but thought I'd push a first draft to see if I've understood the check correctly and so.