Closed rapha-opensource closed 7 years ago
Looked at code. The execution of code in aside() is relying on the fact that the run()
function cancels all remaining tasks when it exits. The raise SystemExit()
in _aside_term()
is forcing a kernel exit, which in term cancels everything else that's remaining. It might make sense to perform a more orderly cancellation though.
Regarding handling of cancellation/exit, it is never really legal to write Curio code that ignores cancellation outright. A task that catches a cancellation should either re-raise the exception or exit in some other way.
Thanks Dave!
Got your point about not ignoring cancellation outright, my example with 'pass' was poor. I pulled the latest version (with the added 'await task.cancel()'), but there's still something I don't understand.
I would expect 'aside' and 'spawn' to behave the same in the following example, namely, 'main' finishes with no traceback.
# saved in test.py
from unittest import TestCase
from curio import run, spawn, aside, sleep
async def main(x):
t = await x(sleep, 30)
await sleep(1)
await t.cancel()
class AsideTest(TestCase):
def test_spawn(self):
# this works just fine, meaning no traceback
run(main(spawn))
def test_aside(self):
# this leaves a SystemExit traceback
run(main(aside))
It seems the SystemExit, which should force a kernel exit, is not caught properly. In Kernel.run (line 171), I see the 'except BaseException' is re-raising the exception but I don't follow where the subsequent cancellations or handling are happening.
I suspect that the traceback you are getting is from Curio's own debugging mechanism which logs task crashes by default. The run() function includes a context manager and a shutdown process that cleans up, even if tasks crash.
Ah, got it. Indeed 'aside' calls 'run' which defaults to having 'logcrash' as its 'debug' arg. I was able to silence the above mentioned traceback by adding the following to my 'aside' coro function:
from logging import getLogger
getLogger('curio.debug').setLevel('CRITICAL')
(I had to use 'CRITICAL' since 'logcrash' defaults to 'ERROR'.)
Would it make sense to add the arguments of 'run' to the 'aside' function, so people can control 'debug' for instance?
Let me think about how I want to handle this. The aside()
functionality is fairly new and experimental. For the sake of debugging, I'm not sure I want crashes to be silent--at least not by default.
Yeah, defaulting to debug makes sense.
The original problem is fixed anyway so maybe we should close this issue.
Thanks for your help Dave!
I could be mistaken, but it seems to me the _aside_term coro function is missing 'task.cancel()' line 23 in curio/curio/side.py
When we cancel an aside task faster than the requested coro can finish, we trip the 'goodbye' event in _aside_term, which immediately raises SystemExit. I would have excepted the requested coro to be given a chance to cancel since the 'aside' doc says SIGTERM is translated into a cancel.
Second question: also, shouldn't we prevent the SystemExit raise if the 'requested coro' task properly handled the CancelledError exception?
Example: