On Mon, Feb 11, 2013 at 12:53 PM, Saúl Ibarra Corretgé <saghul@gmail.com> wrote:

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()?

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.

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?)

    Yeah, that would not work, in my example the task is already started,
    I don't think cancel() will work midway.

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.)

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.

Approved the latter. Gave some detailed feedback on the former; but see my higher-order mumblings above...

--Guido van Rossum (python.org/~guido)