From 8147ce0103eaceb89b0c259dec6de3f955932b78 Mon Sep 17 00:00:00 2001
From: Vincent Ladeuil <vila+ols@canonical.com>
Date: Thu, 21 Apr 2016 11:11:22 +0000
Subject: [PATCH] Make upload more robust by ignoring suprious errors while
 polling the scan status.

While the snap is scanned on the store, the status is polled until completion.

If an error occurs while acquiring the status, ignore the errors.

Those errors are rare and the next retry generally succeeds so ignoring those errors is harmless.

If they persist until the last retry, something else may be going on and this will still be reported.

LP: #1572963
---
 snapcraft/storeapi/_upload.py           | 28 ++++++++++++++++++--
 snapcraft/storeapi/common.py            |  7 -----
 snapcraft/tests/test_storeapi_upload.py | 34 ++++++++++++++++++++++++-
 3 files changed, 59 insertions(+), 10 deletions(-)

diff --git a/snapcraft/storeapi/_upload.py b/snapcraft/storeapi/_upload.py
index 5dbc4fd25a..72a82ff994 100644
--- a/snapcraft/storeapi/_upload.py
+++ b/snapcraft/storeapi/_upload.py
@@ -20,13 +20,13 @@
 import time
 import os
 
+import requests
 from concurrent.futures import ThreadPoolExecutor
 from progressbar import (ProgressBar, Percentage, Bar, AnimatedMarker)
 from requests_toolbelt import (MultipartEncoder, MultipartEncoderMonitor)
 
 from .common import (
     get_oauth_session,
-    is_scan_completed,
     retry,
 )
 from .compat import open, quote_plus, urljoin
@@ -318,6 +318,30 @@ def get_post_files(metadata=None):
     return files
 
 
+def is_scan_completed(response):
+    """Return True if the response indicates the scan process completed."""
+    if response is None:
+        # To cope with spurious connection failures lacking a proper response:
+        # either we'll retry and succeed or we fail for all retries and report
+        # an error.
+        return False
+    if response.ok:
+        return response.json().get('completed', False)
+    return False
+
+
+def get_scan_status(session, url):
+    try:
+        resp = session.get(url)
+        return resp
+    except (requests.ConnectionError, requests.HTTPError):
+        # Something went wrong and we couldn't acquire the status. Upper
+        # level (is_scan_completed) will deal with the None response
+        # meaning we don't know the status. This avoid a spurious
+        # connection error breaking an upload for a wrong reason.
+        return None
+
+
 def get_scan_data(session, status_url):
     """Return metadata about the state of the upload scan process."""
     # initial retry after 5 seconds
@@ -328,7 +352,7 @@ def get_scan_data(session, status_url):
            delay=SCAN_STATUS_POLL_DELAY,
            backoff=1)
     def get_status():
-        return session.get(status_url)
+        return get_scan_status(session, status_url)
 
     response, aborted = get_status()
 
diff --git a/snapcraft/storeapi/common.py b/snapcraft/storeapi/common.py
index 4ee3e23673..8df70ecc27 100644
--- a/snapcraft/storeapi/common.py
+++ b/snapcraft/storeapi/common.py
@@ -67,13 +67,6 @@ def store_api_call(path, session=None, method='GET', data=None):
     return result
 
 
-def is_scan_completed(response):
-    """Return True if the response indicates the scan process completed."""
-    if response.ok:
-        return response.json().get('completed', False)
-    return False
-
-
 def retry(terminator=None, retries=3, delay=3, backoff=2, logger=None):
     """Decorate a function to automatically retry calling it on failure.
 
diff --git a/snapcraft/tests/test_storeapi_upload.py b/snapcraft/tests/test_storeapi_upload.py
index ebe3dd9434..eca672080b 100644
--- a/snapcraft/tests/test_storeapi_upload.py
+++ b/snapcraft/tests/test_storeapi_upload.py
@@ -17,13 +17,20 @@
 import json
 import os
 import tempfile
+import unittest
 
 from mock import ANY, call, patch
-from requests import Response
+from requests import (
+    ConnectionError,
+    HTTPError,
+    Response,
+)
 
 from snapcraft import tests
 from snapcraft.storeapi._upload import (
     get_upload_url,
+    get_scan_status,
+    is_scan_completed,
     upload_app,
     upload_files,
     upload,
@@ -650,3 +657,28 @@ def test_get_upload_url(self):
             upload_url = "http://example.com/click-package-upload/app.dev/"
             url = get_upload_url('app.dev')
             self.assertEqual(url, upload_url)
+
+
+class FakeSession(object):
+
+    def __init__(self, exc):
+        self.exc = exc
+
+    def get(self, url):
+        raise self.exc()
+
+
+class ScanStatusTestCase(unittest.TestCase):
+
+    def test_is_scan_complete_for_none(self):
+        self.assertFalse(is_scan_completed(None))
+
+    def get_scan_status(self, exc):
+        raiser = FakeSession(exc)
+        return get_scan_status(raiser, 'foo')
+
+    def test_get_status_connection_error(self):
+        self.assertIsNone(self.get_scan_status(ConnectionError))
+
+    def test_get_status_http_error(self):
+        self.assertIsNone(self.get_scan_status(HTTPError))