feat: retry upon ChunkedEncodingError#994
feat: retry upon ChunkedEncodingError#994zhijie-yang wants to merge 2 commits intocanonical:mainfrom
Conversation
zhijie-yang
force-pushed
the
retry-in-download-chunks
branch
from
7b80915 to
c576e34
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds retry logic to handle ChunkedEncodingError exceptions during file downloads, implementing automatic retries with progress tracking.
Changes:
- Added retry mechanism with configurable max attempts (3) for download operations
- Implemented progress tracking and rollback functionality for interrupted downloads
- Added comprehensive test coverage for both successful retry scenarios and exhausted retry scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| craft_application/services/request.py | Implements retry logic with progress tracking and rollback for ChunkedEncodingError |
| tests/unit/services/test_request.py | Adds test cases for retry success and retry exhaustion scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert path.read_bytes() == downloads[url] | ||
|
|
||
|
|
||
| def failing_iter_content(chunk_size=None): # pylint: disable=unused-argument |
There was a problem hiding this comment.
The function name 'failing_iter_content' could be more descriptive. Consider renaming to 'simulate_chunked_encoding_error_in_iter_content' to better convey its purpose as a test helper.
|
|
||
| # Patch the get method to simulate ChunkedEncodingError on first attempts | ||
| original_get = request_service.get | ||
| call_count = {"count": 0} |
There was a problem hiding this comment.
Using a dictionary to track a simple counter is unnecessary. Consider using a list with a single element (e.g., call_count = [0]) or a nonlocal variable for cleaner code.
| super().__init__(app, services) | ||
| self._session = requests.Session() | ||
| self._session.headers["User-Agent"] = f"{self._app.name}/{self._app.version}" | ||
| self._max_retries = 3 |
There was a problem hiding this comment.
The max_retries value is hardcoded. Consider making it configurable via constructor parameter or a class constant to improve flexibility and testability.
| # Yield negative sum of chunk sizes to indicate rollback | ||
| yield -downloaded_bytes |
There was a problem hiding this comment.
The rollback mechanism using negative values is not documented in the function's docstring. This behavior should be documented as it's part of the public API contract that callers need to understand.
| except requests.exceptions.ChunkedEncodingError: | ||
| if attempt < self._max_retries - 1: | ||
| craft_cli.emit.progress( | ||
| f"Download interrupted, retrying... (attempt {attempt + 1}/{self._max_retries})" |
There was a problem hiding this comment.
The retry attempt numbering is confusing. When displaying 'attempt 1/3', this is actually the second attempt (first retry). Consider changing to 'retry {attempt + 1} of {self._max_retries - 1}' to accurately reflect retry count rather than total attempts.
| f"Download interrupted, retrying... (attempt {attempt + 1}/{self._max_retries})" | |
| f"Download interrupted, retrying... (retry {attempt + 1} of {self._max_retries - 1})" |
mr-cal
left a comment
mr-cal
left a comment
There was a problem hiding this comment.
Thanks for the PR @zhijie-yang. Can you use craft-application's retry utility to accomplish this?
make lint && make test?docs/reference/changelog.rst)?Resolved #993