🐛 FIX: Task.cancel should not set state as EXCEPTED
#214
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #214 +/- ##
===========================================
- Coverage 90.57% 90.51% -0.05%
===========================================
Files 22 22
Lines 2956 2961 +5
===========================================
+ Hits 2677 2680 +3
- Misses 279 281 +2
Continue to review full report at Codecov.
|
|
The initial commit is the minimum required to fix the aiida-core bug, but actually cc99c5f should be a more "complete" fix |
| @@ -1193,6 +1193,12 @@ async def step(self) -> None: | |||
|
|
|||
| except KeyboardInterrupt: # pylint: disable=try-except-raise | |||
| raise | |||
| except asyncio.CancelledError: # pylint: disable=try-except-raise | |||
There was a problem hiding this comment.
Just wondering: is cancellation conceptually also an interruption or should these two be treated differently?
In the other two cases above you're capturing the CancelledError together with the Interruption.
That makes me wonder whether Interruption should inherit from asyncio.CancelledError and we just generalize _set_interrupt_action_from_exception to deal properly also with that case?
Perhaps a question for @muhrin
There was a problem hiding this comment.
Indeed, it's a bit subtle and it's difficult to give a good answer without looking through in detail all of the code you've been discussing.
However, my intuition is that CancelledError should not be treated as an interruption. My thinking is that, for futures (both concurrent and async), there are three terminal states (success, exception and cancelled) and ideally I would be tempted to keep them as such. Interrupted was created precisely to give another form of intervention for our specific use and, if possible, all interruptions would be channelled through this mechanism.
There was a problem hiding this comment.
thanks @chrisjsewell
Given that we're going to address elsewhere the issue of asyncio.CancelledErrors arriving at this part of the code, the purpose of this PR reduces to making sure the behavior under python3.7 and python3.8 is the same (which is still worthwhile).
plumpy/process_states.py
Outdated
| @@ -316,7 +317,7 @@ def interrupt(self, reason: Any) -> None: | |||
| async def execute(self) -> State: # type: ignore # pylint: disable=invalid-overridden-method | |||
| try: | |||
| result = await self._waiting_future | |||
| except Interruption: | |||
| except (Interruption, asyncio.CancelledError): | |||
There was a problem hiding this comment.
In order to just make behavior under python3.7 the same as it currently is in and python3.8, let's only re-raise for the asyncio.CancelledError (i.e. treat it separately from the Interruption that also manipulates the self._waiting_future)
plumpy/process_states.py
Outdated
| @@ -229,7 +230,7 @@ async def execute(self) -> State: # type: ignore # pylint: disable=invalid-over | |||
| result = self.run_fn(*self.args, **self.kwargs) | |||
| finally: | |||
| self._running = False | |||
| except Interruption: | |||
| except (Interruption, asyncio.CancelledError): | |||
There was a problem hiding this comment.
maybe it makes sense to also here make a separate case for asyncio.CancelledError and add the multi-line comment + add a note that this can be removed once support for python 3.7 is dropped
`asyncio.CancelledError` are generated when an async task is cancelled. In python 3.7 it inherits from `Exception`, whereas in python 3.8+ it inherits from `BaseException`. This meant it python 3.7 it was being caught by the broad `except Exception`, and setting the process state to EXCEPTED, whereas in python 3.8+ it was being re-raised to the caller. We now ensure in both versions it is re-raised.
See aiidateam/aiida-core#4648 (comment)
Currently, stopping the daemon in python 3.7 excepts all processes!