fix: set timeout for requests (#495)

* fix: add timeouts for all HTTP requests * Simplify retryable error messages * reference offset * Some comments on timeouts
mapillary · Jan 15, 2022 · f51fd0f · f51fd0f
1 parent 63bf27d
commit f51fd0f
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 6 deletions.
diff --git a/mapillary_tools/api_v4.py b/mapillary_tools/api_v4.py
@@ -15,13 +15,15 @@
 MAPILLARY_GRAPH_API_ENDPOINT = os.getenv(
     "MAPILLARY_GRAPH_API_ENDPOINT", "https://graph.mapillary.com"
 )
+REQUESTS_TIMEOUT = 60  # 1 minutes
 
 
 def get_upload_token(email: str, password: str) -> requests.Response:
     resp = requests.post(
         f"{MAPILLARY_GRAPH_API_ENDPOINT}/login",
         params={"access_token": MAPILLARY_CLIENT_TOKEN},
         json={"email": email, "password": password, "locale": "en_US"},
+        timeout=REQUESTS_TIMEOUT,
     )
     resp.raise_for_status()
     return resp
@@ -38,6 +40,7 @@ def fetch_organization(
         headers={
             "Authorization": f"OAuth {user_access_token}",
         },
+        timeout=REQUESTS_TIMEOUT,
     )
     resp.raise_for_status()
     return resp
@@ -60,6 +63,7 @@ def logging(
         headers={
             "Authorization": f"OAuth {access_token}",
         },
+        timeout=REQUESTS_TIMEOUT,
     )
     resp.raise_for_status()
     return resp
diff --git a/mapillary_tools/upload_api_v4.py b/mapillary_tools/upload_api_v4.py
@@ -14,7 +14,16 @@
 MAPILLARY_UPLOAD_ENDPOINT = os.getenv(
     "MAPILLARY_UPLOAD_ENDPOINT", "https://rupload.facebook.com/mapillary_public_uploads"
 )
-DEFAULT_CHUNK_SIZE = 1024 * 1024 * 64
+DEFAULT_CHUNK_SIZE = 1024 * 1024 * 16
+REQUESTS_TIMEOUT = 60  # 1 minutes
+# According to the docs, UPLOAD_REQUESTS_TIMEOUT sets both the "connection timeout"
+# and "read timeout": https://docs.python-requests.org/en/latest/user/advanced/#timeouts
+# In my test, however, the connection timeout does not only include the time for connection,
+# but also the time for uploading the actual data.
+# i.e. if your data uploading can't finish in this timeout, it will throw:
+# ConnectionError: ('Connection aborted.', timeout('The write operation timed out'))
+# so make sure the largest possible chunks can be uploaded before this timeout
+UPLOAD_REQUESTS_TIMEOUT = 10 * 60  # 10 minutes
 
 
 FileType = Literal["zip", "mly_blackvue_video"]
@@ -68,7 +77,9 @@ def fetch_offset(self) -> int:
             "Authorization": f"OAuth {self.user_access_token}",
         }
         resp = requests.get(
-            f"{MAPILLARY_UPLOAD_ENDPOINT}/{self.session_key}", headers=headers
+            f"{MAPILLARY_UPLOAD_ENDPOINT}/{self.session_key}",
+            headers=headers,
+            timeout=REQUESTS_TIMEOUT,
         )
         resp.raise_for_status()
         data = resp.json()
@@ -110,6 +121,7 @@ def upload(
                 f"{MAPILLARY_UPLOAD_ENDPOINT}/{self.session_key}",
                 headers=headers,
                 data=chunk,
+                timeout=UPLOAD_REQUESTS_TIMEOUT,
             )
             resp.raise_for_status()
             offset += len(chunk)
@@ -148,6 +160,7 @@ def finish(self, file_handle: str) -> int:
             f"{MAPILLARY_GRAPH_API_ENDPOINT}/finish_upload",
             headers=headers,
             json=data,
+            timeout=REQUESTS_TIMEOUT,
         )
 
         resp.raise_for_status()

diff --git a/mapillary_tools/uploader.py b/mapillary_tools/uploader.py
@@ -380,6 +380,10 @@ def _reset_retries(_, __):
     if emitter:
         emitter.emit("upload_start", mutable_payload)
 
+    MAX_RETRIES = 200
+
+    offset = None
+
     while True:
         fp.seek(0, io.SEEK_SET)
         try:
@@ -396,14 +400,21 @@ def _reset_retries(_, __):
                 fp, chunk_size=chunk_size, offset=offset
             )
         except Exception as ex:
-            if retries < 200 and is_retriable_exception(ex):
+            if retries < MAX_RETRIES and is_retriable_exception(ex):
                 if emitter:
                     emitter.emit("upload_interrupted", mutable_payload)
                 retries += 1
                 sleep_for = min(2 ** retries, 16)
                 LOG.warning(
-                    f"Error uploading, resuming in {sleep_for} seconds",
-                    exc_info=True,
+                    # use %s instead of %d because offset could be None
+                    f"Error uploading chunk_size %d at offset %s: %s: %s",
+                    chunk_size,
+                    offset,
+                    ex.__class__.__name__,
+                    str(ex),
+                )
+                LOG.info(
+                    "Retrying in %d seconds (%d/%d)", sleep_for, retries, MAX_RETRIES
                 )
                 time.sleep(sleep_for)
             else:

diff --git a/requirements.txt b/requirements.txt
@@ -3,7 +3,7 @@ piexif==1.1.3
 gpxpy==1.4.2
 pymp4==1.1.0
 pynmea2==1.12.0
-requests==2.20.0
+requests>=2.20.0,<3.0.0
 tqdm>=4.0,<5.0
 typing_extensions
 jsonschema