Add retry logic for streaming timeout errors in Urllib3Fetcher #2896
Conversation
Co-authored-by: Ishaan Gupta <ishaankone@gmail.com> Signed-off-by: Hemang Sharrma <hemangsharrma@gmail.com>
Thanks, I should have time for this next week. In the mean time, could you talk about how you tested that this really works and whether there is a way to test in our test suite? the whole download code is now pretty complicated (and an absolutely core part of the software) so I'd feel more sure about this if it was thoroughly tested... With regards to the failing lint: You can run |
tuf/ngclient/urllib3_fetcher.py
Outdated
|
|
||
| self._proxy_env = ProxyEnvironment(headers={"User-Agent": ua}) | ||
| # Configure retry strategy: retry on read timeouts and connection errors | ||
| # This enables retries for streaming failures, not just initial connection |
There was a problem hiding this comment.
am I correct that retry_strategy variable does not actually affect the streaming retries at all? it seems to be a completely separate implementation
There was a problem hiding this comment.
this is useful if the connection fails before streaming, although this shouldn't be a part of this PR, I could remove it
There was a problem hiding this comment.
Sure, it seems to be codifying the default urllib3 behaviour here. I'm asking about the comment that seems to claim that setting retry_strategy enables retries for streaming failures
Co-authored-by: Vedant Mahajan <vedant.04.mahajan@gmail.com> Signed-off-by: Hemang Sharrma <hemangsharrma@gmail.com>
If you want to see the tests run on CI you can lower the --fail-under limit in |
…comment Signed-off-by: Hemang Sharrma <hemangsharrma@gmail.com>
|
@jku Thank you for the feedback. The comment on retry_strategy variable was misleading as it only handles retries before streaming begins. I fixed the comment and added 5 comprehensive tests to test_fetcher_ng.py, these test the case where urllib3's built-in retries don't help because the failure happens after the HTTP request completes. Coverage should now exceed 97%. |
4 participants
Collaborated with @hemang1404
Description of the changes being introduced by the pull request:
Configure urllib3 Retry with 3 retries for read/connect timeouts
Fixes #2842
@jku please have a look and if there are any changes please let us know