Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 FIX: Task.cancel should not set state as EXCEPTED #214

Merged
merged 6 commits into from
Mar 8, 2021
Merged

🐛 FIX: Task.cancel should not set state as EXCEPTED #214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
43f403e
🐛 FIX: `Task.cancel` should not set state as EXCEPTED
chrisjsewell Mar 4, 2021
cc99c5f
Add CancelledError re-raise also in process states execution
chrisjsewell Mar 4, 2021
0cc754f
fix pre-commit
chrisjsewell Mar 4, 2021
6e8d2a2
apply reveiw suggestion
chrisjsewell Mar 8, 2021
498824f
Update plumpy/process_states.py
chrisjsewell Mar 8, 2021
afbe291
fix pre-commit
chrisjsewell Mar 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions plumpy/process_states.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# -*- coding: utf-8 -*-
import asyncio
from enum import Enum
import sys
import traceback
Expand Down Expand Up @@ -232,6 +233,11 @@ async def execute(self) -> State: # type: ignore # pylint: disable=invalid-over
except Interruption:
# Let this bubble up to the caller
raise
except asyncio.CancelledError: # pylint: disable=try-except-raise
# note this re-raise is only required in python<=3.7,
# for python>=3.8 asyncio.CancelledError does not inherit from Exception,
# so will not be caught below
raise
except Exception: # pylint: disable=broad-except
excepted = self.create_state(ProcessState.EXCEPTED, *sys.exc_info()[1:])
return cast(State, excepted)
Expand Down
6 changes: 6 additions & 0 deletions plumpy/processes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@ltalirz ltalirz Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# note this re-raise is only required in python<=3.7,
# where asyncio.CancelledError == concurrent.futures.CancelledError
# it is encountered when the run_task is cancelled
# for python>=3.8 asyncio.CancelledError does not inherit from Exception, so will not be caught below
raise
except Exception: # pylint: disable=broad-except
# Overwrite the next state to go to excepted directly
next_state = self.create_state(process_states.ProcessState.EXCEPTED, *sys.exc_info()[1:])
Expand Down