Fix Haaaanger on Download (#20)

* fix: ConnectionResetError haaaaanger on downlaod, closes #19
* fix: don't hang on unhandled exceptions in download
* chore: clean up docker-compose.yaml

docker-compose.yaml

@@ -1,14 +1,5 @@
 version: '3.4'
 services:
-  upparat:
-    build: .
-    image: upparat:local
-    command: --verbose
-    volumes:
-      # make sure to not mount over .egginfo in /app/upparat
-      - ./src/upparat:/app/upparat/src/upparat
-      - ./setup.py:/app/upparat/setup.py
-      - ./setup.cfg:/app/upparat/setup.cfg
   test:
     build: .
     image: upparat:local

src/upparat/statemachine/download.py

@@ -36,10 +36,18 @@
 REQUEST_TIMEOUT_SEC = 30
 BACKOFF_EXPO_MAX_SEC = 2 ** 6  # 64
 
+RETRYABLE_EXCEPTIONS = (
+    URLError,
+    HTTPError,
+    RemoteDisconnected,
+    socket.timeout,
+    ConnectionResetError,
+)
+
 
 @backoff.on_exception(
     functools.partial(backoff.expo, max_value=BACKOFF_EXPO_MAX_SEC),
-    (URLError, HTTPError, RemoteDisconnected, socket.timeout),
+    RETRYABLE_EXCEPTIONS,
     jitter=backoff.full_jitter,
 )
 def download(job, stop_download, publish, update_job_progress):
@@ -111,6 +119,18 @@ def download(job, stop_download, publish, update_job_progress):
         else:
             logger.error(f"HTTPError {http_error.status}: {http_error.reason}.")
             raise http_error
+    except Exception as exception:
+        if type(exception) not in RETRYABLE_EXCEPTIONS:
+            # see issue #19, instead of hanging on unhanded exception,
+            # we try to improve the situation by just starting over
+            # since we don't have any better error recovery strategy.
+            logger.exception("Unhandled failure. Starting over.")
+            update_job_progress(JobProgressStatus.DOWNLOAD_INTERRUPT.value)
+            publish(pysm.Event(DOWNLOAD_INTERRUPTED))
+        else:
+            # let backoff.on_exception handle retry
+            # for this exception
+            raise exception
 
 
 class DownloadState(JobProcessingState):

tests/statemachine/download_test.py

@@ -119,6 +119,7 @@ def test_download_interrupted_on_http_403(mocker, download_state, urllib_urlopen
         ([b"11", RemoteDisconnected(), b"22", b"33", b""], "112233"),
         ([b"11", URLError("reason"), b"22", b"33", b""], "112233"),
         ([b"11", socket.timeout(), b"22", b"33", b""], "112233"),
+        ([b"11", ConnectionResetError(), b"22", b"33", b""], "112233"),
     ],
 )
 def test_download_completed_successfully_with_retries(
@@ -155,6 +156,31 @@ def test_download_completed_successfully_with_retries(
     assert second_request.full_url == state.job.file_url
 
 
+@pytest.mark.parametrize(
+    "urlopen_side_effect, expected_download",
+    [([b"11", Exception("What could go wrong?"), b"22", b"33", b""], "112233")],
+)
+def test_download_interrupted_on_exception(
+    urlopen_side_effect,
+    expected_download,
+    urllib_urlopen_mock,
+    download_state,
+    mocker,
+    tmpdir,
+):
+    urlopen_mock = urllib_urlopen_mock(side_effect=urlopen_side_effect)
+    mocker.patch("urllib.request.urlopen", urlopen_mock)
+    mocker.patch("time.sleep")  # to make test faster
+
+    state, inbox, mqtt_client, _, _ = download_state
+    state.on_enter(None, None)
+
+    # check if download gets interrupted
+    # and thus retried, see issue #19.
+    event = inbox.get(timeout=TIMEOUT)
+    assert event.name == DOWNLOAD_INTERRUPTED
+
+
 def test_download_job_progress_updates(mocker, download_state, urllib_urlopen_mock):
     urlopen_side_effect = [b"11", b"22", b"33", b""]
     urlopen_mock = urllib_urlopen_mock(side_effect=urlopen_side_effect)