On Mon, Feb 11, 2013 at 12:53 PM, Saúl Ibarra Corretgé email@example.com:
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...