Cancel and fail downloads if exception happened #2710
Conversation
| except Exception as ex: | ||
| self._logger.error(f"Extractor failed with an error: {ex}") | ||
| lazy_downloads.cancel() | ||
| raise | ||
| finally: | ||
| # wait for all downloads to be finished | ||
| await lazy_downloads.join() |
There was a problem hiding this comment.
I couldn't find a clear answer on this; is the finally block still called if an error is raised in the caught exception block?
There was a problem hiding this comment.
Yup, see this for example:
There was a problem hiding this comment.
In that case, the finally block is awaiting all of the downloads even if there's an error. But I guess lazy_downloads.cancel() is wiping everything, so that's okay?
(Asking because the PR title is Cancel and fail downloads if exception happened but it looks like it will still eventually await the downloads).
There was a problem hiding this comment.
Yeah it's intended:
Cancellation does not mean that the task is immediately dead - we need to make sure that the task has an opportunity to gracefully cancel.
For example if the cancelled task catches asyncio.CancelledError and does something, then it will still need to be awaited.
We use similar logic here:
connectors/connectors/services/base.py
Lines 177 to 182 in 8b8cb0d
task.cancelI() injects the throw of asyncio.CancelledError where it can (pretty much any await or async with statement). After that the code can catch it or not. The except part here is checking that task re-raised asyncio.CancelledError cause it could not handle it - there was no except asyncio.CancelledError inside task or this error was re-raised.
Hope the description makes sense, if not I can prepare an illustrative example of this :)
There was a problem hiding this comment.
That's really helpful, thanks!
artem-shelkovnikov
deleted the
artem/cancel-download-tasks-if-something-goes-wrong
branch
💔 Failed to create backport PR(s)The backport operation could not be completed due to the following error: The backport PRs will be merged automatically after passing CI. To backport manually run: |
💔 Failed to create backport PR(s)The backport operation could not be completed due to the following error: The backport PRs will be merged automatically after passing CI. To backport manually run: |
Solving problems found during CI runs:
If something goes wrong when Extractor is extracting the documents, all the scheduled downloads are still finished. That could mean that if there's a queue of 1000 documents waiting to be processed, even if the job fails all 1000 documents will be downloaded and extracted.
Additionally, there's no error checking for failed downloads - if a download fails, connector keeps working. Even if all downloads fail, connector still keeps working.
We instead want to cancel the syncs in the queue.
This PR will be followed up by #2671: it'll be possible to set up error thresholds for this logic.
Checklists
Pre-Review Checklist
config.yml.example)v7.13.2,v7.14.0,v8.0.0)Release Note