Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: add retries for 'requests.ConnectionError' #186

Merged
merged 7 commits into from Dec 3, 2020
Merged

fix: add retries for 'requests.ConnectionError' #186

merged 7 commits into from Dec 3, 2020

Conversation

andrewsg
Copy link
Contributor

Restructures retry strategy to check both status codes and exceptions; then, adds requests.ConnectionError as a retriable exception. (Note: the unified standard library ConnectionError class is not used because it is unsupported by both Requests and Python 2).

@andrewsg andrewsg requested a review from a team as a code owner November 16, 2020 19:09
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 16, 2020
tseaver
tseaver previously requested changes Nov 16, 2020
View reviewed changes
google/resumable_media/_helpers.py Outdated Show resolved Hide resolved
tritone
tritone reviewed Nov 16, 2020
View reviewed changes
google/resumable_media/_helpers.py Outdated Show resolved Hide resolved
google/resumable_media/_helpers.py Show resolved Hide resolved
google/resumable_media/_helpers.py Outdated Show resolved Hide resolved
google/_async_resumable_media/_helpers.py Outdated Show resolved Hide resolved
google/resumable_media/_helpers.py Outdated Show resolved Hide resolved
tests/unit/test__helpers.py Outdated Show resolved Hide resolved
@tseaver tseaver changed the title Retry requests.ConnectionError fix: add retries for 'requests.ConnectionError' Nov 16, 2020
@ahill00
Copy link

ahill00 commented Nov 19, 2020

Is it possible that this would also retry requests that have ChunkedEncodingError?

Seeing this in google-resumable-media==1.10 (python 2.7, but I imagine other versions are impacted):

    response = download.consume(transport, timeout=timeout)
  File "/usr/local/lib/python2.7/dist-packages/google/resumable_media/requests/download.py", line 171, in consume
    self._write_to_stream(result)
  File "/usr/local/lib/python2.7/dist-packages/google/resumable_media/requests/download.py", line 105, in _write_to_stream
    for chunk in body_iter:
  File "/usr/local/lib/python2.7/dist-packages/requests/models.py", line 756, in generate
    raise ChunkedEncodingError(e)
ChunkedEncodingError: ("Connection broken: error(104, 'Connection reset by peer')", error(104, 'Connection reset by peer'))```

@andrewsg
Copy link
Contributor Author

@tritone @tseaver Coverage at 100% now, PTAL. Also I'd like your opinion on @andyhky's suggestion above regarding ChunkedEncodingError. My feeling is we should treat it as a ConnectionError here, do you agree? If so I'll add it to our supported exceptions.

frankyn
frankyn requested changes Nov 24, 2020
View reviewed changes
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, just have nits on commentary.

google/resumable_media/_helpers.py Outdated
def _get_connection_error_class():
"""Get the exception error class.

This is a separate function for testing purposes."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify "testing purposes" with detecting if requests is being used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, good to explain this layer of indirection.

google/resumable_media/_helpers.py

# Set the retriable_exception_type if possible. We expect requests to be
# present here and the transport to be using requests.exceptions errors,
# but due to loose coupling with the transport layer we can't guarantee it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you file an issue to track potentially improving this in the future? E.g. Reliance on having a specific transport requests.

tritone
tritone approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

google/resumable_media/_helpers.py Outdated
def _get_connection_error_class():
"""Get the exception error class.

This is a separate function for testing purposes."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, good to explain this layer of indirection.

@andrewsg andrewsg requested a review from tseaver December 3, 2020 17:29
@andrewsg andrewsg dismissed tseaver’s stale review December 3, 2020 17:32

Feedback addressed, have two other reviews. Thanks!

@andrewsg andrewsg merged commit 0d76eac into master Dec 3, 2020
@andrewsg andrewsg deleted the retry branch December 3, 2020 17:35
@tschaub
Copy link

tschaub commented Dec 5, 2020

Thanks for the work on this and for adding retries in the case of ChunkedEncodingError.

We're seeing quite a few ChunkedEncodingError: ("Connection broken: error(104, 'Connection reset by peer')", error(104, 'Connection reset by peer')) failures while paging through bucket.list_blobs() queries to GCS, and hoping that retries on ChunkedEncodingError will improve the situation.

Are there regularly scheduled releases of this package? Or is there another way we can get a sense for when this change might be released?

@andrewsg
Copy link
Contributor Author

@tschaub Working on a release today.

@andrewsg
Copy link
Contributor Author

Correction, it's Friday so we'll target Monday.

@tswast tswast mentioned this pull request Dec 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankyn frankyn frankyn approved these changes

@tritone tritone tritone approved these changes

@tseaver tseaver Awaiting requested review from tseaver

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@andrewsg @ahill00 @tschaub @tseaver @frankyn @tritone