chore: better handle CancelledError from static analysis #275
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of failing (and raising) in the case of a request exception (in `send_single_upload_put`) we will now catch the exception, emit warning, and return safely. This is important because the aggregate of all tasks WILL fail if one of the requests fail. Also notice that `httpx.HTTPError` is the base error class for a number of request-related errors, so this covers many of the failure possibilities here
We have seen stacktraces of static analysis failing with `asyncio.exceptions.CancelledError: Cancelled by cancel scope 7f7514b8ffa0` I do believe that the initial cause is one of the requests failing, and that cancels every other task, but there's a still possible scenario in which the outer future (`asyncio.gather itself`) gets cancelled. Assuming that it happens and that we do get the same exception bubble up, these changes would let the program exit (more) gracefully. Maybe there are other exceptions there that we should also look for?
giovanni-guidini
requested review from
dana-yaish,
joseph-sentry,
matt-codecov and
scott-codecov
Codecov Report
@@ Coverage Diff @@
## master #275 +/- ##
==========================================
+ Coverage 94.91% 95.34% +0.42%
==========================================
Files 78 78
Lines 2696 2708 +12
==========================================
+ Hits 2559 2582 +23
+ Misses 137 126 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
|
joseph-sentry
approved these changes
Oct 4, 2023
| "succeeded": True, | ||
| } | ||
| if response.status_code in retryable_statuses: | ||
| await asyncio.sleep(2**current_retry) |
There was a problem hiding this comment.
It'd be good to write a test that covers this line but otherwise lgtm
giovanni-guidini
deleted the
gio/handle-static-analysis-put-failures-better
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
context: codecov/engineering-team#643
Currently static analysis command is still very delicate and breaks easily.
Not so much (anymore) in the analysis part, but the uploading of files, specially in the first few runs, can cause indecipherable stack traces that simply state
CancelledErrorwas thrown.This PR introduces some error handling for
send_single_upload_putfunction and the general gathering of many such tasks in the hopes that we can fail gracefully (and not land on your face)