PEP 3156 / Tulip: stopping the loop while in run_until_complete
Hi, While analyzing some code I came across the following situation: - User calls run_until_complete for some Task - The task itself calls loop.stop() at some point - run_until_complete raises TimeoutError Here is a very simple example: https://gist.github.com/saghul/4754117 Something seems a bit off here. While one could argue that stopping the loop while run_until_complete is ongoing, getting a TimeoutError doesn't feel right. I think we should detect that the loop was stopped but the future is not done and raise something like NotCompleted or FutureNotCompleted. Thoughts? -- Saúl Ibarra Corretgé http://saghul.net/blog | http://about.me/saghul
Yeah, I think the base implementation of run_until_complete is wrong. I think I didn't have stop() when I coded that. Could you change it to something that just registers a lambda calling stop() on the Future, and see if that makes your test case behave better? (If so, please add a unit test and submit for code review.) On Mon, Feb 11, 2013 at 4:21 AM, Saúl Ibarra Corretgé <saghul@gmail.com> wrote:
-- --Guido van Rossum (python.org/~guido)
Guido van Rossum wrote:
Hum, I guess I'm missing something, but futures don't have a stop() method. The code already calls loop.stop() when the future is done. What I had in mind was to add a _called to Handler and expose it with a readonly property, so then we could check if handler.called and raise TimeoutError or not. As for raising another exception, maybe it's not such a good idea, since caller can always check future.done() later anyway. -- Saúl Ibarra Corretgé http://saghul.net/blog | http://about.me/saghul
On Mon, Feb 11, 2013 at 10:18 AM, Saúl Ibarra Corretgé <saghul@gmail.com>wrote:
Oops, I read your post (and the code) too fast.
I'm not so keen on that. But you were probably calling run_until_complete() without an explicit timeout. In that case, it should not call run() but run_forever(), and it should never raise TimeoutError. You could still get a TimeoutError if a timeout was given; in that case, I think you can fix this case by passing call_later() a helper function that sets a nonlocal variable which you then inspect. You could also use a different mechanism, e.g. call cancel() on a Future when the timeout occurs. (But I think that might be less reliable, since I think Tasks can catch cancellations.) -- --Guido van Rossum (python.org/~guido)
Right, this was actually the case. I'll submit a code review addressing this.
Sure, but then what should we do: return like nothing has happened or raise an exception so that the caller knows that the future is not complete?
Yeah, that would not work, in my example the task is already started, I don't think cancel() will work midway. -- /Saúl http://saghul.net | http://sipdoc.net
On Mon, Feb 11, 2013 at 11:29 AM, Saúl Ibarra Corretgé <saghul@gmail.com>wrote:
Cool.
The same thing as when no timeout is specified. I presume this means returning None. The alternative would be to call future.result() in all cases -- it would raise InvalidStateError, but that doesn't feel very useful. Perhaps this is actually the correct response? What is the reason you have something else that calls stop()?
Actually, it may. There is code to throw CancelledError into the generator, in _step(). But I don't think it will necessarily break out of all blocking I/O. (Though it really should -- perhaps this is something useful to add tests for.) -- --Guido van Rossum (python.org/~guido)
I didn't run into this in practice, I just thought of it :-) I guess that some exception handler or some third party lib could do it without you knowing, but I don't have a good example on why someone would want to do it.
Well, it may work for tasks, but since run_until_complete applies to futures in general I'm not sure if that would still work. I added the following review which fixes the problem and adds a test case for it: https://codereview.appspot.com/7301076/ A few days ago I also submitted this one: https://codereview.appspot.com/7312046/ which adds a few more tests for run_until_complete. Regards, -- Saúl Ibarra Corretgé http://saghul.net/blog | http://about.me/saghul
On Mon, Feb 11, 2013 at 12:53 PM, Saúl Ibarra Corretgé <saghul@gmail.com>wrote:
I think there are actually two cases: where stop() is called from the task given to run_until_complete(), or from another task. In the former case, because the exception bubbles out of the task's coroutine, the task will be marked as done (and its exception set to _StopError) -- at least I think so. In the latter case, the task will still be runnable, and it will continue to run when the event loop is started again. (Hm... maybe run_forever() should be called start(), to match stop()?) I think your unittest only tests the former case. I'm not sure what run_until_complete() should return in the latter case. In fact it makes me doubt the behavior of run_until_complete() in general. Maybe it should return the Future (although that's kind of redundant)? Or an enum indicating what happened? (Cases would be result, exception, timeout, running -- and maybe cancelled?)
Approved the latter. Gave some detailed feedback on the former; but see my higher-order mumblings above... -- --Guido van Rossum (python.org/~guido)
Well, I think they are actually the same. In the gist I posted earlier I called loop.stop(), I didn't raise the exception inside the task, and loop.stop() will call call_soon so the actual raising will happen in the context on _run_once unless I'm mistaken. Here is an updated simple test with it's output: https://gist.github.com/saghul/4758151
I think the current behavior is ok, it the user really wants to check if the future was actually completed she can check with done(), if she saved the future in a variable, that is. My other preferred approach would be to raise an exception, since I consider this an exceptional case: I told the loop to run until this future is completed so I expect that to happen. Since it didn't, NotCompletedError (or something alike) sounds good to me. -- Saúl Ibarra Corretgé http://saghul.net/blog | http://about.me/saghul
On Mon, Feb 11, 2013 at 2:23 PM, Saúl Ibarra Corretgé <saghul@gmail.com>wrote:
Oh, you're right! Not my day for reading carefully. :-)
Good. How about calling future.result(), and letting it raise InvalidStateError ? (Except if the timeout handler was called.) -- --Guido van Rossum (python.org/~guido)
Good. How about calling future.result(), and letting it raise InvalidStateError ? (Except if the timeout handler was called.)
I did a quick test to see how it feels and I do like it. I'll update the review with this change plus the other suggested ones. -- Saúl Ibarra Corretgé http://saghul.net/blog | http://about.me/saghul
Yeah, I think the base implementation of run_until_complete is wrong. I think I didn't have stop() when I coded that. Could you change it to something that just registers a lambda calling stop() on the Future, and see if that makes your test case behave better? (If so, please add a unit test and submit for code review.) On Mon, Feb 11, 2013 at 4:21 AM, Saúl Ibarra Corretgé <saghul@gmail.com> wrote:
-- --Guido van Rossum (python.org/~guido)
Guido van Rossum wrote:
Hum, I guess I'm missing something, but futures don't have a stop() method. The code already calls loop.stop() when the future is done. What I had in mind was to add a _called to Handler and expose it with a readonly property, so then we could check if handler.called and raise TimeoutError or not. As for raising another exception, maybe it's not such a good idea, since caller can always check future.done() later anyway. -- Saúl Ibarra Corretgé http://saghul.net/blog | http://about.me/saghul
On Mon, Feb 11, 2013 at 10:18 AM, Saúl Ibarra Corretgé <saghul@gmail.com>wrote:
Oops, I read your post (and the code) too fast.
I'm not so keen on that. But you were probably calling run_until_complete() without an explicit timeout. In that case, it should not call run() but run_forever(), and it should never raise TimeoutError. You could still get a TimeoutError if a timeout was given; in that case, I think you can fix this case by passing call_later() a helper function that sets a nonlocal variable which you then inspect. You could also use a different mechanism, e.g. call cancel() on a Future when the timeout occurs. (But I think that might be less reliable, since I think Tasks can catch cancellations.) -- --Guido van Rossum (python.org/~guido)
Right, this was actually the case. I'll submit a code review addressing this.
Sure, but then what should we do: return like nothing has happened or raise an exception so that the caller knows that the future is not complete?
Yeah, that would not work, in my example the task is already started, I don't think cancel() will work midway. -- /Saúl http://saghul.net | http://sipdoc.net
On Mon, Feb 11, 2013 at 11:29 AM, Saúl Ibarra Corretgé <saghul@gmail.com>wrote:
Cool.
The same thing as when no timeout is specified. I presume this means returning None. The alternative would be to call future.result() in all cases -- it would raise InvalidStateError, but that doesn't feel very useful. Perhaps this is actually the correct response? What is the reason you have something else that calls stop()?
Actually, it may. There is code to throw CancelledError into the generator, in _step(). But I don't think it will necessarily break out of all blocking I/O. (Though it really should -- perhaps this is something useful to add tests for.) -- --Guido van Rossum (python.org/~guido)
I didn't run into this in practice, I just thought of it :-) I guess that some exception handler or some third party lib could do it without you knowing, but I don't have a good example on why someone would want to do it.
Well, it may work for tasks, but since run_until_complete applies to futures in general I'm not sure if that would still work. I added the following review which fixes the problem and adds a test case for it: https://codereview.appspot.com/7301076/ A few days ago I also submitted this one: https://codereview.appspot.com/7312046/ which adds a few more tests for run_until_complete. Regards, -- Saúl Ibarra Corretgé http://saghul.net/blog | http://about.me/saghul
On Mon, Feb 11, 2013 at 12:53 PM, Saúl Ibarra Corretgé <saghul@gmail.com>wrote:
I think there are actually two cases: where stop() is called from the task given to run_until_complete(), or from another task. In the former case, because the exception bubbles out of the task's coroutine, the task will be marked as done (and its exception set to _StopError) -- at least I think so. In the latter case, the task will still be runnable, and it will continue to run when the event loop is started again. (Hm... maybe run_forever() should be called start(), to match stop()?) I think your unittest only tests the former case. I'm not sure what run_until_complete() should return in the latter case. In fact it makes me doubt the behavior of run_until_complete() in general. Maybe it should return the Future (although that's kind of redundant)? Or an enum indicating what happened? (Cases would be result, exception, timeout, running -- and maybe cancelled?)
Approved the latter. Gave some detailed feedback on the former; but see my higher-order mumblings above... -- --Guido van Rossum (python.org/~guido)
Well, I think they are actually the same. In the gist I posted earlier I called loop.stop(), I didn't raise the exception inside the task, and loop.stop() will call call_soon so the actual raising will happen in the context on _run_once unless I'm mistaken. Here is an updated simple test with it's output: https://gist.github.com/saghul/4758151
I think the current behavior is ok, it the user really wants to check if the future was actually completed she can check with done(), if she saved the future in a variable, that is. My other preferred approach would be to raise an exception, since I consider this an exceptional case: I told the loop to run until this future is completed so I expect that to happen. Since it didn't, NotCompletedError (or something alike) sounds good to me. -- Saúl Ibarra Corretgé http://saghul.net/blog | http://about.me/saghul
On Mon, Feb 11, 2013 at 2:23 PM, Saúl Ibarra Corretgé <saghul@gmail.com>wrote:
Oh, you're right! Not my day for reading carefully. :-)
Good. How about calling future.result(), and letting it raise InvalidStateError ? (Except if the timeout handler was called.) -- --Guido van Rossum (python.org/~guido)
Good. How about calling future.result(), and letting it raise InvalidStateError ? (Except if the timeout handler was called.)
I did a quick test to see how it feels and I do like it. I'll update the review with this change plus the other suggested ones. -- Saúl Ibarra Corretgé http://saghul.net/blog | http://about.me/saghul
participants (2)
-
Guido van Rossum -
Saúl Ibarra Corretgé