Activity worker: refactoring part 2 #899
Conversation
dandavison
force-pushed
the
activity-worker-refactor-2
branch
from
0fde874 to
8213e5e
Compare
dandavison
force-pushed
the
activity-worker-refactor-2
branch
from
8213e5e to
8d3f066
Compare
temporalio/worker/_activity.py
Outdated
| async def wait_all_completed(self) -> None: | ||
| running_tasks = [v.task for v in self._running_activities.values() if v.task] | ||
| if running_tasks: | ||
| await asyncio.gather(*running_tasks, return_exceptions=False) | ||
| await asyncio.gather(*running_tasks, return_exceptions=True) |
There was a problem hiding this comment.
This makes sense to me (we don't want to return until all tasks are complete) except now this swallows exceptions. Has this logic been hit and is raising somehow today? I suspect we did mean to have this be return_exceptions=True originally, just want to confirm whether there is something observed precipitating this change.
There was a problem hiding this comment.
Let's leave this for a PR focused specifically on changing that. Perhaps we want to consume the stream and re-raise any exceptions encountered. We'd discussed this previously at #860 (comment) and concluded that we felt it should be changed. Also I'm going to want the Nexus worker to do the thing we think is right. But, I've reverted this change: let's merge this PR with the pure refactoring (it will keep activity and nexus workers consistent) and kick this issue down the road one more time.
There was a problem hiding this comment.
Works for me. May want to change PR title/description.
dandavison
force-pushed
the
activity-worker-refactor-2
branch
from
8d3f066 to
f13fdc6
Compare
dandavison
changed the title
wait_all_completed + refactoring
dandavison
force-pushed
the
activity-worker-refactor-2
branch
from
f13fdc6 to
ea31487
Compare
wait_all_completed