Fix failure to shutdown on keyboard interrupt #3982
Conversation
| except KeyboardInterrupt as exc: | ||
| try: | ||
| await scheduler.shutdown(exc) | ||
| except Exception as exc2: | ||
| # In case of exceptions in the shutdown method itself. | ||
| LOG.exception(exc2) | ||
| raise exc2 from None |
There was a problem hiding this comment.
No need for this anymore as the shutdown is handled in scheduler.run() or scheduler.start_scheduler()
cylc/flow/scheduler.py
Outdated
| @@ -643,7 +643,7 @@ async def run(self): | |||
| await self.configure() | |||
| await self.start_servers() | |||
| await self.log_start() | |||
| except Exception as exc: | |||
| except (KeyboardInterrupt, asyncio.CancelledError, Exception) as exc: | |||
| await self.shutdown(exc) | |||
There was a problem hiding this comment.
Might want to copy over the try/except to ensure that shutdown traceback gets dumped to the flow log if not handled by scheduler_cli.
There was a problem hiding this comment.
@MetRonnie two approvals, but will let you reply this one before I merge it (happy if you or @oliver-sanders merge it after you've taken a look on this one too 👍 )
There was a problem hiding this comment.
Works for me, tested 3.7/3.8.
(I'm not sure we can test this?)
It's do-able via bash though fiddly, we used to do this for cylc scan (tests/functional/cylc-scan/02-sigstop.t).
Should be able to do via Python though, could try writing an integration test and raising KeyboardInterrupt from within the Scheduler, not 100% sure what would happen but worth a try:
reg = flow(one_conf)
schd = scheduler(reg)
async with run(schd) as flow_log:
# await asyncio.sleep(0) # try this if it doesn't work
raise KeyboardInterrupt()
assert flow_log.record_tuples[-1] == [...]
There was a problem hiding this comment.
Worked like a charm. Switched to this branch, re-installed my venv, started five. Stopped it, and it exited nicely 👍
Thanks @MetRonnie !
|
Having a go at the integration test |
|
Couldn't get the integration test to work. It seemed like the |
| elif isinstance(reason, SchedulerError): | ||
| LOG.error('Suite shutting down - %s', reason) | ||
| LOG.error(f'Suite shutting down - {reason}') |
There was a problem hiding this comment.
Works for me too; thanks @MetRonnie 👍
I'll merge this now, a test for this isn't super-critical. We developers are likely to notice quickly if this breaks in future.
These changes close #3946
Ctrl+C was failing to shutdown the scheduler on Python 3.8+ due to the fact that
asyncio.CanclledErrorchanged from subclassingExceptiontoBaseException. See https://stackoverflow.com/a/60167684/3217306 and https://docs.python.org/3.8/library/asyncio-exceptions.html#asyncio.CancelledErrorI'm new to asyncio, and there seems to be another way of handling signal interruptions: https://www.roguelynn.com/words/asyncio-graceful-shutdowns/. However, the changes I've made are very simple and seem to work.
Requirements check-list
CONTRIBUTING.mdand added my name as a Code Contributor.