Improve retrying downloads code and testing #1559

yarikoptic · 2024-12-20T21:54:16Z

Follow up to

Continue retrying downloads on retriable statuses #1558
retry on 429 as well
handle Retry-After as a Date not just seconds (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date)
adjust logging of sleeping upon retry - do in a single place

We are into holidays already, so it is understood if review does not come. I will reserve the "right" to merge if there is user demand but likely we could just wait. If anyone sees "clearly" how to improve -- feel welcome to push directly.

…leep

… attempts

codecov · 2024-12-20T21:57:10Z

Codecov Report

Attention: Patch coverage is 98.59155% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.47%. Comparing base (bb05a23) to head (6b57379).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
dandi/download.py	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
- Coverage   88.53%   88.47%   -0.07%     
==========================================
  Files          78       78              
  Lines       10735    10798      +63     
==========================================
+ Hits         9504     9553      +49     
- Misses       1231     1245      +14

Flag	Coverage Δ
unittests	`88.47% <98.59%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jwodder · 2024-12-20T21:57:36Z

dandi/consts.py

@@ -160,7 +160,7 @@ def urls(self) -> Iterator[str]:

 #: HTTP response status codes that should always be retried (until we run out
 #: of retries)
-RETRY_STATUSES = (500, 502, 503, 504)
+RETRY_STATUSES = (429, 500, 502, 503, 504)


Adding 429 here will also affect requests made by dandiapi.py and dandiarchive.py. Unless you want to add support for Retry-After there as well, it'd be better to just add 429 next to RETRY_STATUSES in download.py.

jwodder · 2024-12-20T21:58:12Z

dandi/download.py

@@ -3,7 +3,8 @@
 from collections import Counter, deque
 from collections.abc import Callable, Iterable, Iterator, Sequence
 from dataclasses import InitVar, dataclass, field
-from datetime import datetime
+from datetime import UTC, datetime


datetime.UTC requires Python 3.11. Use datetime.timezone.utc instead.

dandi/download.py

jwodder · 2024-12-20T22:01:04Z

dandi/download.py

-    # if is_access_denied(exc) or attempt >= 2:
-    #     raise
-    # sleep a little and retry
-    else:


By removing this else, the lgr.debug() call on line 1138/1156 will be followed by the call on line 1157/1167, resulting in two similar log messages.

i think it would only be in the case of non-conformant "retry-after" we would get 3 messages "on the matter"

"to response %d with non-integer or future date"

"to response %d with Retry-After"

the one below.

if there is a good Retry-after, we would get a non-negative value for sleep_amount, and log only at 1138/1156 (but we might want it into else: for exception, to not log with /1147) as sleep_amount condition on /1164 would not trigger that other message where we would also define the random sleep.

But for above "3 msgs" I will adjust... thanks!

candleindark · 2024-12-27T07:37:49Z

dandi/download.py

+            if retry_after.isdigit():
+                sleep_amount = int(retry_after)


If we are assuming that this client always connect to our trusted DANDI API, I think this is fine, but if we want to want to play even "safer" I think we may want to ensure the retry_after string is really a non-negative integer in a decimal expression.

If retry_after is "²³" will crash this client at this point.

This safe guard can be excessive though since we have control of both the client and server.

I think use of str.isdecimal() and ensuring the return of int() to be non-negative is better.

candleindark

At this point, _check_if_more_attempts_allowed is equally, if not more, about sleeping/waiting before sending out another requests. I think it would be beneficial to rename it, or in some other way, to indicate this nature.

If we want to improve the retrying further, I think we can consider incorporating some well-known and recommended algorithms for retrying requests to a server while avoiding overwhelming it. https://chatgpt.com/share/676f8290-86cc-800a-9aa8-1ad952282d27.

candleindark · 2024-12-28T04:26:39Z

dandi/download.py

+                try:
+                    retry_after_date = parsedate_to_datetime(retry_after)
+                    current_date = datetime.now(UTC)
+                    difference = retry_after_date - current_date
+                    sleep_amount = int(difference.total_seconds())
+                    if sleep_amount < 0:
+                        lgr.warning(
+                            "%s - date in Retry-After=%r is in the past (current is %r)",
+                            path,
+                            retry_after,
+                            current_date,
+                        )
+                except ValueError as exc_ve:
+                    # our code or response is wrong, do not crash but issue warning
+                    # and continue with "default" sleep logic
+                    lgr.warning(
+                        "%s - download failed due to response %d with non-integer or future date"
+                        " Retry-After=%r: %s with %s upon converting assuming it is a date",
+                        path,
+                        exc.response.status_code,
+                        retry_after,
+                        exc,
+                        exc_ve,
+                    )


Suggested change

try:

retry_after_date = parsedate_to_datetime(retry_after)

current_date = datetime.now(UTC)

difference = retry_after_date - current_date

sleep_amount = int(difference.total_seconds())

if sleep_amount < 0:

lgr.warning(

"%s - date in Retry-After=%r is in the past (current is %r)",

path,

retry_after,

current_date,

)

except ValueError as exc_ve:

# our code or response is wrong, do not crash but issue warning

# and continue with "default" sleep logic

lgr.warning(

"%s - download failed due to response %d with non-integer or future date"

" Retry-After=%r: %s with %s upon converting assuming it is a date",

path,

exc.response.status_code,

retry_after,

exc,

exc_ve,

)

try:

retry_after_date = parsedate_to_datetime(retry_after)

except (ValueError, TypeError) as exc_ve:

# our code or response is wrong, do not crash but issue warning

# and continue with "default" sleep logic

lgr.warning(

"%s - download failed due to response %d with non-integer or future date"

" Retry-After=%r: %s with %s upon converting assuming it is a date",

path,

exc.response.status_code,

retry_after,

exc,

exc_ve,

)

else:

current_date = datetime.now(timezone.utc)

difference = retry_after_date - current_date

sleep_amount = int(difference.total_seconds())

if sleep_amount < 0: # todo: `sleep_amount < 0` may not be an error. It the server set a `Retry-After` datetime extremely close and the network is slow, the client may receive the response after the `Retry-After` datetime.

lgr.warning(

"%s - date in Retry-After=%r is in the past (current is %r)",

path,

retry_after,

current_date,

)

This suggestion make the try clause

more targeted

catches TypeErrors from parsedate_to_datetime. (Try parsedate_to_datetime('Wed, 21 O3t 2015 07:280 GMT'))

adds a note with a todo comment regarding sleep_amount < 0

candleindark · 2024-12-28T04:28:35Z

dandi/download.py

+                        retry_after,
+                        exc,
+                        exc_ve,
+                    )
            lgr.debug(
                "%s - download failed due to response %d with "
                "Retry-After=%d: %s, will sleep and retry",


the "Retry-After=[]" in this log is now insistent and sleep_amount may not be a meaningful number if retry_after failed to be parsed into a datetime object

candleindark · 2024-12-28T04:32:39Z

dandi/download.py

@@ -1122,19 +1123,36 @@ def _check_if_more_attempts_allowed(
            return None
        elif retry_after := exc.response.headers.get("Retry-After"):


This is not about the changes in this PR. However, since are trying to improve this function, we may as well consider replace elif with if here. The last if clause ended with a return statement, so the elif here is about confusing.

So can be said about the containing elif clause at line 1111 at which I can't place a comment.

candleindark · 2024-12-28T04:36:30Z

dandi/download.py

-    # sleep a little and retry
-    else:
+    if sleep_amount < 0:
+        # it was not Retry-after set, so we come up with random duration to sleep


In the current arrangement, the execution flow can reach here event if Retry-After is set. It can be set to an extremely close datetime or some string can't be parsed properly in to a datetime object

yarikoptic added 3 commits December 20, 2024 15:34

Simplify: a single log.debug invocation for any case which leads to s…

4cc65c2

…leep

Retry-After is provided by 429 (too many) so retry on that too

88f3901

Handle Retry-After as a date, and add tests for all logic of checking…

6b57379

… attempts

yarikoptic added the patch Increment the patch version when merged label Dec 20, 2024

yarikoptic requested review from jwodder, asmacdo and candleindark December 20, 2024 21:54

jwodder requested changes Dec 20, 2024

View reviewed changes

candleindark reviewed Dec 27, 2024

View reviewed changes

candleindark reviewed Dec 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve retrying downloads code and testing #1559

Improve retrying downloads code and testing #1559

yarikoptic commented Dec 20, 2024

codecov bot commented Dec 20, 2024 •

edited

Loading

jwodder Dec 20, 2024

jwodder Dec 20, 2024

jwodder Dec 20, 2024

yarikoptic Dec 20, 2024

candleindark Dec 27, 2024

candleindark left a comment

candleindark Dec 28, 2024

candleindark Dec 28, 2024

candleindark Dec 28, 2024

candleindark Dec 28, 2024

candleindark Dec 28, 2024

		@@ -1122,19 +1123,36 @@ def _check_if_more_attempts_allowed(
		return None
		elif retry_after := exc.response.headers.get("Retry-After"):

Improve retrying downloads code and testing #1559

Are you sure you want to change the base?

Improve retrying downloads code and testing #1559

Conversation

yarikoptic commented Dec 20, 2024

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candleindark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 20, 2024 •

edited

Loading