[SYNPY-1514] Handle Expired Pre-Signed URLs

[BWMac]

Problem:

Due to the unordered nature of AsyncIO executing tasks async, when there is a large queue of files to download, the file may not be downloaded for a while after a pre-signed URL is retrieved. This shows up as a 403 Forbidden error which needs to be handled for these cases to prevent downloads from failing.

Solution:

Update download_from_url to handle these 403 responses and try again by checking if the URL is expired when we receive a 403 response code, retrieving a new URL and retrying the download.

Testing:

• This was tested by:
  1. Reproducing the error faced by a user by using the random package to have this failure occur.
  2. Hard-coding an expired URL into the code and running a synapse get CLI command
• new unit and integration tests were added

[pep8speaks]

Hello @BWMac! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file synapseclient/core/download/download_functions.py:

Line 744:89: E501 line too long (93 > 88 characters)
Line 756:89: E501 line too long (103 > 88 characters)
Line 776:89: E501 line too long (97 > 88 characters)
Line 788:89: E501 line too long (121 > 88 characters)

• In the file tests/integration/synapseclient/core/test_download.py:

Line 21:1: F401 'synapseclient.core.exceptions' imported but unused
Line 23:1: F401 'synapseclient.core.exceptions.SynapseHTTPError' imported but unused

• In the file tests/unit/synapseclient/core/unit_test_download.py:

Line 680:89: E501 line too long (90 > 88 characters)
Line 725:89: E501 line too long (92 > 88 characters)
Line 860:89: E501 line too long (93 > 88 characters)
Line 863:89: E501 line too long (90 > 88 characters)
Line 925:89: E501 line too long (93 > 88 characters)
Line 928:89: E501 line too long (90 > 88 characters)

Comment last updated at 2024-09-03 22:11:44 UTC

[BryanFauble]

There was also an SSL exception that was in the logs the user had provided. Can you also see if we can add that to the exceptions that are retried?

[BWMac]

@BryanFauble We definitely could, but as written my solution to the 403 response codes is to check if the URL is expired and then if it is, retry. Will the URL also be expired in the case where SSLZeroReturnError is raised or should this case bypass the URL check/refresh?

[BryanFauble]

@BryanFauble We definitely could, but as written my solution to the 403 response codes is to check if the URL is expired and then if it is, retry. Will the URL also be expired in the case where SSLZeroReturnError is raised or should this case bypass the URL check/refresh?

I don't think a 403 is going to be raised. I think the solution is to add it to the standard retry params:

synapsePythonClient/synapseclient/client.py

Line 190 in 2dda954

"retry_exceptions": RETRYABLE_CONNECTION_EXCEPTIONS,

In the retry function you can also pass the actual exception class too. I think we will want to add it as one of the things to retry, just like the connection exceptions that are raised. That way the ssl errors are treated with the same 5 minute retry logic.

[BryanFauble]

Great changes! Just a few small areas that changes could be made, but shouldn't affect overall intention of the code.

[BryanFauble]

LGTM!

[thomasyu888]

🔥 LGTM! This is neat, although the exception handling definitely looks more complex now.