-
Notifications
You must be signed in to change notification settings - Fork 241
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
Making timeout_after/ignore_after truly composable #82
Comments
Hmmm. I'm going to have to wrap my brain around this whole scenario, but I think I see where you're going here... maybe. Regarding the name "TaskTimeout".... Python already defines a built-in exception |
Having slept on it, I think I have a slightly better idea for how to organize the exceptions... For the "inner" code, the timeout is a signal imposed from outside saying "you have to stop now", and the only correct way to handle it is to clean up and return ASAP. This is exactly the same rule that code should use when it gets cancelled. Basically from the point of the code that has the timeout imposed on it, a timeout is a cancellation, just a particular kind. So probably it should be possible to distinguish between "you got timed out" and "you got manually canceled", but the default should be to handle them the same. So let's enhance And then the exception that gets raised from |
One general concern on nesting is just what happens when you go beyond 2 levels. The two-exception idea definitely gets crazy then. The idea of making the timeout error subclass CancelledError seems reasonable though. That could probably work. Let me sleep on it. |
No, it's totally fine I think? [btw probably no need for ableist language there?] This is why you need the "cookies" I was talking about so that for a given exception you can tell which
If timeout A fires, then code4->code3->code2 see Hmm, I guess one possible source of confusion here is that in the current code, I think the code jumps through some hoops so that if A's deadline is before B's, then B's deadline gets adjusted earlier so that B "has a chance" to fire? I think it's simpler to think of it as, each |
...Though on further thought actually, that does leave a tricky question about what to do with code like: async with timeout_after(5):
async with timeout_after(10):
try:
await curio.sleep(100) # after 5 seconds, outer timeout expires
finally:
# while unwinding the outer timeout, we yield and the *inner* timeout fires
await curio.sleep(100) This could get nasty, because in the naive implementation where all timeouts fire when ready, then the inner block's exception could fire second and end up overwriting the outer block's exception. Which would be bad -- the whole point of I think this means that the right semantics are: whenever an "outer" async with timeout_after(10):
async with timeout_after(5):
# note: *not* curio.sleep, simulating a coroutine that doesn't enter the kernel for a while
time.sleep(20)
# now when we enter the kernel and trigger a timeout check, both timeouts have expired
await curio.sleep(1) Here it was possible in principle for the inner timeout to fire first, but as things turned out they fired simultaneously, so the outer one should override the inner one. Summing up, I think something like this gives the desired semantics. One nice thing is that it turns out that because we have ruled out inner timeouts firing after outer timeouts, we don't need to tag exceptions with unique cookies after all -- a simple nesting depth suffices: # logic somewhere inside the kernel used for firing timeouts
if task._deadline_stack[-1] is not None and now > task._deadline_stack[-1]:
# some timeout is firing, but which?
nesting_depth = 0
while now > task._deadline_stack[-1]:
nesting_depth += 1
task._deadline_stack.pop()
exc = CancelledDueToTimeout()
exc._nesting_depth = nesting_depth
coro.throw(exc)
class timeout_after:
async def __aenter__(self):
task = await curio.current_task()
deadline_stack = task._deadline_stack
# hack: if our deadline is too far in the future, it can never fire, but it's simplest if we always
# push something onto the stack. So we duplicate the topmost deadline, and then if/when
# we fire then the timeout below us will also fire at the same time, and override us.
deadline = self.deadline
if deadline is not None and deadline_stack[-1] < deadline:
deadline = deadline_stack[-1]
deadline_stack.append(deadline)
async def __aexit__(self, type, exc, traceback):
if exc is None:
(await curio.current_task())._deadline_stack.pop()
return
elif isinstance(exc, CancelledDueToTimeout):
exc._nesting_depth -= 1
if exc._nesting_depth == 0:
# we're the timeout_after that triggered this cancellation, and the code we
# were wrapping has all been canceled. So convert to a timeout exception.
raise CurioTimeoutError from exc
# otherwise, let the exception continue propagating (possibly with reduced _nesting_depth) [2016-10-16T02:34: made some edits to the code at the bottom to properly pop the deadline stack on clean exits and to support using the |
An idea: What if the TaskTimeout exception included a boolean flag to indicate whether or not the specified time period has actually expired? For example:
Making something like this work would be a whole lot easier than managing two different exception types, an internal stack of timeouts, or anything too complicated. Code could obviously ignore the expired flag and bail out for any kind of timeout. However, if you needed to know if the inner timeout expired and not any other, this would tell you (if the inner timeout was not expired, it means the timeout is due to an outer timeout of some kind). |
So this is a flag on the exception that would have a different value depending on whether you catch it inside or outside the triggering timeout_after block? How are you planning to arrange that? Or am I misunderstanding the proposal? |
Every timeout in Curio is initiated by the timeout_after() function. I'm proposing that any TaskTimeout exception that propagates out of it have an expired flag to indicate whether or not the timeout period given to timeout_after() has expired or not. One of my general concerns is that timeout handling is already quite tricky. I'm somewhat hesitant to make it even more tricky. However, putting a simple boolean flag on the exception to indicate whether or not the specified time interval has expired seems like it would be rather straightforward. It would also solve the initial problem of knowing if an inner timeout has expired or not. |
I just looked at the code and adding this expired flag turned out to be far easier than I thought (involving just a couple of lines of code). I've added it and pushed the changes. Maybe worth experimenting with. |
Oh, I see, so the flag tells you whether you are outside at least one expired But unfortunately, the semantics I want are that I want to know whether I am inside at least one expired code1
async with timeout_after(5):
code2
async with timeout_after(10):
code3 Your flag lets me distinguish the regions {code1+code2} versus {code3}, right? But what we have here is that {code2+code3} have timed out and need to exit, but code1 has not. It might be helpful to go back to my example at the very top of the thread and think about how you would make it work? I don't see how your flag makes it possible to fix that code, but I could be wrong. |
Wouldn't the first example turn into this?
Key thing: If you're getting a timeout and the timeout period has not expired, the timeout must have originated elsewhere. The only elsewhere (in this case) is from the outer timeout. |
One Note: Each timeout_after() is guaranteed to generate one TaskTimeout--even if nested and even if the timeouts occur at the same time. It's possible that both the inner and outer timeout would expire at the same time in this code. However, it would generate two exceptions. The first would cause a cycle back to the next URL. The second timeout would occur almost instantly after that and cause the except block to bail out to the outer function (since the expired flag would now be False). |
Ah, right, yeah -- when exactly one timeout has fired, then "outside exactly one
...ah, github just live-updated with your followup and I see you saying what I was just working out myself about the cycling back and re-firing :-). Okay, I think I understand. On first impression I'm not a big fan, for two reasons:
But I'll ponder and see if I can reconcile myself to it... |
Hmm, actually, I guess my second objection could be fixed by combining your check-for-expiration-en-passant approach with my two-exception-types proposal. So you'd have a I'm still uncomfortable with the idea that adding a |
Just a note that various "footgun" problems with timeouts are addressed in the docs. https://curio.readthedocs.io/en/latest/devel.html#timeouts I'll need to ponder this more though.... |
After sleeping on it, I think I'm coming around to the two-exception idea. Will look at it later today. |
I'm probably going to regret ever suggesting this, but I'm now thinking that three exceptions might be needed here. Consider this scenario:
The inner timeout will fire and create a I'm somewhat inclined to say no. Instead, maybe it should receive some kind of operational error Short version: You will only get a |
Pushed a change that addresses this with exceptions. Some details in the CHANGES file. Have also updated the docs. I think this is cool--solves a lot of tricky corner cases. |
Potentially useful reference: a discussion in twisted about how to handle timeouts in the context of |
Interesting. I wonder if Curio's TaskTimeout should subclass CancellationError. Maybe. |
I expect there will be more fine-tuning of curio's timeout apis as we gain more experience with them :-) Some lower-priority things that will probably be important to add eventually; might as well write them down here so that they don't (or at least, might not :-)) get lost:
|
I like these ideas. I recently changed curio to use absolute clocks internally. So, having an |
Ugh, and here's another nasty case that we should be aware of. With async generator support coming in, consider the effect of: async def ugh():
async with curio.timeout_after(10):
yield "have fun"
async def blargh():
await ugh().__anext__()
# ugh's timeout fires here, entirely outside of the with block
await curio.sleep(20) Probably there is no good solution right now except maybe to detect the worst cases and give some better-than-nothing error message. For the python 3.7 timeframe something like PEP 521 might be the solution... |
Oh, that's kind of evil. I'm not exactly sure what a programmer writing that would expect the behavior to be since there's no way to actually know when control will return from a yield (if at all). I'm not even sure how I'd detect something like that. |
I guess we could at least detect when a timeout exception manages to On Nov 8, 2016 2:11 AM, "David Beazley" notifications@github.com wrote:
|
Closing this for now. |
As you know I'm a big fan on
timeout_after
. The awesome thing about it is that it's composable, in the sense that I can treat any random coroutine as a black box and slap atimeout_after
around it to impose a timeout on it, so long as the called coroutine follows some simple rules. Basically, it should be written so that my timeout fires, then it promptly cleans up and exits. Not an onerous constraint.Except... it's hard to follow this rule in coroutines that themselves want to use
timeout_after
. The docs allude to some of this; here's an example where everything goes wrong because the internal coroutine can't tell whether it's the internal timeout that fired, or the outer one:I suggest that the key missing feature is that when we see a timeout exception, we need to be able to figure out whether it's one that means that we have timed out and should clean up and let our caller get on with things, versus one that means a thing we called has timed out and we should recover however makes sense (e.g. by falling back to trying another server). Are we inside the
timeout_after
that fired, or outside?Proposal: arrange for the inner code to see a different exception type than the outer code.
Implementation: give each
timeout_after
a magic cookie, and when a timeout fires, inject an instance of the "inner" exception type with the magic cookie attached. When an exception passes through atimeout_after
, check if the cookies match, and if so, convert to the "outer" exception type. (Andignore_after
should check the cookie, and swallow the exception iff it matches.)When there are stacked timeouts that fire simultaneously, e.g.
then it should always be the outermost timeout whose cookie is used (so the line at
-- mark --
should see an "inner" exception, not an "outer" one).Unsolved problem: I am drawing a blank on what to call these two exception types to distinguish them :-(. OperationTimeOut versus CancelledTimeOut? [Edit: or maybe ExternalTimeout / InternalTimeout? CallerTimeout / CalleeTimeout? OutsideTimeout / InsideTimeout? LocalTimeout?]
(Speaking of timeout exception names, why is it called
TaskTimeout
? It doesn't really have anything to do with Tasks in particular, does it? I mean, any more so than literally everything else in curio?)The text was updated successfully, but these errors were encountered: