NickAkhmetov/Implement error handling for API timeouts and improve connection management

CHANGELOG-api-client-timeouts.md

@@ -0,0 +1,3 @@
+- Show a 504 Gateway Timeout page, instead of a 500 error, when an upstream API times out; show 502 or 503 pages when an upstream API is unreachable or unavailable.
+- Reuse connections to the search, entity, and assets APIs across requests, and stop requests to them from hanging indefinitely.
+- Update portal-visualization to 0.5.14, which reuses connections and sets timeouts on its requests to the search, entity, and assets APIs.

context/app/main.py

@@ -56,6 +56,16 @@ def forbidden(e):
     return render_react_error(403, 'Forbidden')
 
 
+def bad_gateway(e):
+    """A 502 means an API could not be reached or returned an invalid response."""
+    return render_react_error(502, 'Bad Gateway')
+
+
+def service_unavailable(e):
+    """A 503 means an API is temporarily unavailable."""
+    return render_react_error(503, 'Service Unavailable')
+
+
 def gateway_timeout(e):
     """A 504 means the API has timed out."""
     return render_react_error(504, 'Gateway Timeout')
@@ -121,6 +131,8 @@ def create_app(testing=False):
     app.register_error_handler(401, unauthorized)
     app.register_error_handler(403, forbidden)
     app.register_error_handler(404, not_found)
+    app.register_error_handler(502, bad_gateway)
+    app.register_error_handler(503, service_unavailable)
     app.register_error_handler(504, gateway_timeout)
     app.register_error_handler(500, any_other_error)
 

context/app/routes_api.py

@@ -12,6 +12,7 @@
     make_blueprint,
     get_client,
     get_default_flask_data,
+    EXTERNAL_REQUEST_TIMEOUT,
 )
 
 
@@ -299,5 +300,7 @@ def _dicts_to_tsv(data_dicts, first_fields, descriptions_dict=None):
 def get_globus_groups():
     # The globus auth helper from hubmap_commons omits the group descriptions,
     # so we need to fetch the full list of groups from the repo.
-    groups = requests.get(current_app.config['GLOBUS_GROUPS_URL']).json()
+    groups = requests.get(
+        current_app.config['GLOBUS_GROUPS_URL'], timeout=EXTERNAL_REQUEST_TIMEOUT
+    ).json()
     return groups

context/app/routes_auth.py

@@ -7,7 +7,7 @@
 from json import dumps
 from hubmap_commons.hm_auth import AuthHelper
 
-from .utils import make_blueprint
+from .utils import EXTERNAL_REQUEST_TIMEOUT, make_blueprint
 
 
 # This is mostly copy-and-paste from
@@ -33,7 +33,11 @@ def get_globus_groups(groups_token):
         'Authorization': 'Bearer ' + groups_token,
     }
 
-    response = requests.get('https://groups.api.globus.org/v2/groups/my_groups', headers=headers)
+    response = requests.get(
+        'https://groups.api.globus.org/v2/groups/my_groups',
+        headers=headers,
+        timeout=EXTERNAL_REQUEST_TIMEOUT,
+    )
 
     response.raise_for_status()
     groups = response.json()
@@ -393,7 +397,9 @@ def login():
     log('6: userinfo')
     user_info_request_headers = {'Authorization': 'Bearer ' + auth_token}
     user_info = requests.get(
-        'https://auth.globus.org/v2/oauth2/userinfo', headers=user_info_request_headers
+        'https://auth.globus.org/v2/oauth2/userinfo',
+        headers=user_info_request_headers,
+        timeout=EXTERNAL_REQUEST_TIMEOUT,
     ).json()
 
     # Extract user information
@@ -423,7 +429,9 @@ def login():
         # - If you're logged in, you should be logged in all the way... Easier to debug.
         workspaces_post_url = current_app.config['WORKSPACES_ENDPOINT'] + '/tokens/'
         workspaces_post_data = dumps({'auth_token': groups_token})
-        workspaces_post_resp = requests.post(workspaces_post_url, data=workspaces_post_data)
+        workspaces_post_resp = requests.post(
+            workspaces_post_url, data=workspaces_post_data, timeout=EXTERNAL_REQUEST_TIMEOUT
+        )
 
         try:
             workspaces_token = workspaces_post_resp.json()['token']

context/app/routes_browse.py

@@ -8,8 +8,8 @@
 from urllib.parse import urlparse, quote
 from xml.sax.saxutils import escape as xml_escape
 
-# Private, but so is the `_request` this module already relies on; both come from the
-# portal-visualization client that portal-ui pins.
+# `_paginate_search_after` is private, but it comes from the portal-visualization client
+# that portal-ui pins.
 from portal_visualization.client import HEAVY_RELATIVE_FIELDS, _paginate_search_after
 
 from .utils import _load_tutorials, get_organs, get_valid_tutorial_routes
@@ -707,7 +707,7 @@ def _get_publication_data_types_and_organs(uuid: str):
 
     data_types, organs = [[], []]
     try:
-        response = client._request(elasticsearch_url, request)
+        response = client.request(elasticsearch_url, request)
         aggregations = response.get('aggregations')
         data_types = [
             bucket.get('key') for bucket in aggregations.get('data_types').get('buckets')
@@ -792,7 +792,7 @@ def _get_sitemap_entities_cached(entity_type, hour_bucket):
     }
     try:
         hits = _paginate_search_after(
-            lambda body: client._request(elasticsearch_url, body),
+            lambda body: client.request(elasticsearch_url, body),
             query,
             description=f'{entity_type} sitemap',
         )
@@ -816,7 +816,7 @@ def _get_all_template_keys(hour_bucket):
     client = get_client()
     templates_url = current_app.config['USER_TEMPLATES_ENDPOINT'] + '/templates/jupyter_lab'
     try:
-        return list(client._request(templates_url)['data'].keys())
+        return list(client.request(templates_url)['data'].keys())
     except Exception as e:
         # A `finally: return response_json['data'].keys()` here swallowed the request error
         # and then raised KeyError, taking down the whole sitemap. Losing the template pages
@@ -1063,7 +1063,7 @@ def _get_related_publication_citations(entity):
         ],
     }
     try:
-        response = client._request(elasticsearch_url, query)
+        response = client.request(elasticsearch_url, query)
         hits = response.get('hits', {}).get('hits', [])
     except Exception as e:
         # The dataset page must still render, and the rest of the LD is still worth serving,

context/app/static/js/pages/Error/Error.spec.tsx

@@ -18,6 +18,8 @@ describe('Error page', () => {
     [401, 'Unauthorized'],
     [403, 'Access Denied'],
     [404, 'Page Not Found'],
+    [502, 'Bad Gateway'],
+    [503, 'Service Unavailable'],
     [504, 'Gateway Timeout'],
     [500, 'Internal Server Error'],
   ])('%i error page displays correct titles %s', (errorCode, title) => {
@@ -46,12 +48,12 @@ describe('Error page', () => {
   it('should display titles for unexpected error codes', () => {
     mockGetErrorTitleAndSubtitle.mockReturnValue({
       title: 'Unexpected Error',
-      subtitle: 'HTTP Error 502',
+      subtitle: 'HTTP Error 418',
     });
 
-    render(<Error errorCode={502} />);
+    render(<Error errorCode={418} />);
     expect(screen.getByText('Unexpected Error')).toBeInTheDocument();
-    expect(screen.getByText('502', { exact: false })).toBeInTheDocument();
+    expect(screen.getByText('418', { exact: false })).toBeInTheDocument();
   });
 
   it('should display titles and message for error boundary', () => {

context/app/static/js/pages/Error/utils.ts

@@ -3,6 +3,8 @@ const errorTitle: Record<number, string> = {
   401: 'Unauthorized',
   403: 'Access Denied',
   404: 'Page Not Found',
+  502: 'Bad Gateway',
+  503: 'Service Unavailable',
   504: 'Gateway Timeout',
   500: 'Internal Server Error',
 };

context/app/test_routes_api.py

@@ -74,7 +74,7 @@ def client():
 def mock_es_post(path, **kwargs):
     class MockResponse:
         def __init__(self):
-            self.status_code = 0  # _request requires a status code
+            self.status_code = 0  # ApiClient.request requires a status code
             self.text = 'Logger call requires this'
 
         def json(self):
@@ -94,7 +94,7 @@ def mock_es_get(path, **kwargs):
 
     class MockResponse:
         def __init__(self):
-            self.status_code = 0  # _request requires a status code
+            self.status_code = 0  # ApiClient.request requires a status code
             self.text = 'Logger call requires this'
 
         def json(self):
@@ -117,21 +117,21 @@ def tsv_assertions(response):
 
 
 def test_tsv_get(client, mocker):
-    mocker.patch('requests.post', side_effect=mock_es_post)
-    mocker.patch('requests.get', side_effect=mock_es_get)
+    mocker.patch('requests.Session.post', side_effect=mock_es_post)
+    mocker.patch('requests.Session.get', side_effect=mock_es_get)
     response = client.get('/metadata/v0/donors.tsv')
     tsv_assertions(response)
 
 
 def test_tsv_post(client, mocker):
-    mocker.patch('requests.post', side_effect=mock_es_post)
-    mocker.patch('requests.get', side_effect=mock_es_get)
+    mocker.patch('requests.Session.post', side_effect=mock_es_post)
+    mocker.patch('requests.Session.get', side_effect=mock_es_get)
     response = client.post('/metadata/v0/donors.tsv', json={'uuids': []})
     tsv_assertions(response)
 
 
 def test_unexpected_json_tsv_post(client, mocker):
-    mocker.patch('requests.post', side_effect=mock_es_post)
+    mocker.patch('requests.Session.post', side_effect=mock_es_post)
     response = client.post('/metadata/v0/donors.tsv', json={'unexpected': []})
     assert response.status == '200 OK'  # TODO: Should not be 200!
     # https://github.com/hubmapconsortium/portal-ui/issues/2348
@@ -142,7 +142,7 @@ def test_unexpected_json_tsv_post(client, mocker):
 
 
 def test_unexpected_args_tsv_post(client, mocker):
-    mocker.patch('requests.post', side_effect=mock_es_post)
+    mocker.patch('requests.Session.post', side_effect=mock_es_post)
     response = client.post('/metadata/v0/donors.tsv?hello=world')
     assert response.status == '200 OK'  # TODO: Should not be 200!
     # https://github.com/hubmapconsortium/portal-ui/issues/2348

context/app/test_routes_browse.py

@@ -47,7 +47,7 @@ def raise_for_status(self):
 def app_context(mocker):
     # _get_dataset_ld reads request.base_url for the `url` field, and queries Elasticsearch
     # for related publications. Default to none, so citation-specific tests opt in.
-    mocker.patch('requests.post', side_effect=mock_publications_post([]))
+    mocker.patch('requests.Session.post', side_effect=mock_publications_post([]))
     app = create_app(testing=True)
     with app.test_request_context('/browse/dataset/fake-uuid?redirected=True'):
         yield
@@ -148,7 +148,7 @@ def test_citation_holds_related_publications(mocker):
     "whenever possible", so each snippet ends in the publication's DOI.
     """
     mocker.patch(
-        'requests.post',
+        'requests.Session.post',
         side_effect=mock_publications_post(
             [
                 {
@@ -183,7 +183,7 @@ def test_citation_query_covers_processed_descendants(mocker):
     A publication may reference a processed descendant rather than the raw dataset, so the
     lookup has to match on the dataset and every descendant.
     """
-    post = mocker.patch('requests.post', side_effect=mock_publications_post([]))
+    post = mocker.patch('requests.Session.post', side_effect=mock_publications_post([]))
     app = create_app(testing=True)
     with app.test_request_context('/browse/dataset/fake-uuid'):
         _get_dataset_ld({**DATASET, 'descendant_ids': ['processed-uuid', 'support-uuid']})
@@ -199,7 +199,7 @@ def test_citation_query_covers_processed_descendants(mocker):
 
 def test_citation_survives_a_failed_publication_lookup(mocker):
     """The rest of the LD is still worth serving if the publication query fails."""
-    mocker.patch('requests.post', side_effect=Exception('Elasticsearch is down'))
+    mocker.patch('requests.Session.post', side_effect=Exception('Elasticsearch is down'))
     app = create_app(testing=True)
     with app.test_request_context('/browse/dataset/fake-uuid'):
         ld = _get_dataset_ld(DATASET)
@@ -355,8 +355,8 @@ def test_funder_ror_is_the_common_fund(app_context):
 def test_canonical_link_drops_query_string(client, mocker):
     from .test_routes_main import mock_prov_get, mock_search_donor_post
 
-    mocker.patch('requests.get', side_effect=mock_prov_get)
-    mocker.patch('requests.post', side_effect=mock_search_donor_post)
+    mocker.patch('requests.Session.get', side_effect=mock_prov_get)
+    mocker.patch('requests.Session.post', side_effect=mock_search_donor_post)
     response = client.get('/browse/donor/fake-uuid?redirected=True&redirectedFromId=HBM999')
     html = response.data.decode('utf8')
     assert '<link rel="canonical" href="http://localhost/browse/donor/fake-uuid">' in html
@@ -367,8 +367,8 @@ def test_no_ld_script_for_non_datasets(client, mocker):
     # tag is decided server-side, not by the bundle.
     from .test_routes_main import mock_prov_get, mock_search_donor_post
 
-    mocker.patch('requests.get', side_effect=mock_prov_get)
-    mocker.patch('requests.post', side_effect=mock_search_donor_post)
+    mocker.patch('requests.Session.get', side_effect=mock_prov_get)
+    mocker.patch('requests.Session.post', side_effect=mock_search_donor_post)
     response = client.get('/browse/donor/fake-uuid')
     assert 'application/ld+json' not in response.data.decode('utf8')
 
@@ -404,8 +404,8 @@ def capture(path, **kwargs):
         bodies.append(kwargs.get('json'))
         return mock_search_dataset_post(path, **kwargs)
 
-    mocker.patch('requests.get', side_effect=mock_prov_get)
-    mocker.patch('requests.post', side_effect=capture)
+    mocker.patch('requests.Session.get', side_effect=mock_prov_get)
+    mocker.patch('requests.Session.post', side_effect=capture)
 
     client.get('/browse/dataset/fake-uuid')
     entity_queries = [b for b in bodies if b and 'ids' in str(b.get('query', ''))]
@@ -426,8 +426,8 @@ def test_dataset_ld_is_in_the_served_html(client, mocker):
     """
     from .test_routes_main import mock_prov_get
 
-    mocker.patch('requests.get', side_effect=mock_prov_get)
-    mocker.patch('requests.post', side_effect=mock_search_dataset_post)
+    mocker.patch('requests.Session.get', side_effect=mock_prov_get)
+    mocker.patch('requests.Session.post', side_effect=mock_search_dataset_post)
     response = client.get('/browse/dataset/fake-uuid')
     assert response.status == '200 OK'
     html = response.data.decode('utf8')
@@ -502,7 +502,7 @@ def test_processed_dataset_redirects_permanently(client, mocker):
     permanent. A 302 would leave each processed URL canonical in Google's eyes and eligible
     for indexing as a near-duplicate of the primary dataset page.
     """
-    mocker.patch('requests.post', side_effect=mock_processed_dataset_post)
+    mocker.patch('requests.Session.post', side_effect=mock_processed_dataset_post)
     response = client.get('/browse/dataset/processed-uuid')
     assert response.status == '301 MOVED PERMANENTLY'
     assert response.location.startswith('/browse/dataset/raw-uuid')
@@ -513,7 +513,7 @@ def test_sitemap_index(client, mocker):
     from .routes_browse import _get_sitemap_entities_cached
 
     _get_sitemap_entities_cached.cache_clear()
-    mocker.patch('requests.post', side_effect=mock_sitemap_search_post)
+    mocker.patch('requests.Session.post', side_effect=mock_sitemap_search_post)
     response = client.get('/sitemap.xml')
     _get_sitemap_entities_cached.cache_clear()
 
@@ -573,7 +573,7 @@ def test_sitemap_entity_includes_lastmod(client, mocker):
 
     # The lookup is memoized per hour bucket; clear it so this test sees the mock.
     _get_sitemap_entities_cached.cache_clear()
-    mocker.patch('requests.post', side_effect=mock_sitemap_search_post)
+    mocker.patch('requests.Session.post', side_effect=mock_sitemap_search_post)
     response = client.get('/sitemap-dataset.xml')
     _get_sitemap_entities_cached.cache_clear()
 
@@ -593,7 +593,7 @@ def test_sitemap_lists_only_datasets_with_their_own_page(client, mocker):
     from .routes_browse import _get_sitemap_entities_cached
 
     _get_sitemap_entities_cached.cache_clear()
-    post = mocker.patch('requests.post', side_effect=mock_sitemap_search_post)
+    post = mocker.patch('requests.Session.post', side_effect=mock_sitemap_search_post)
     client.get('/sitemap-dataset.xml')
     _get_sitemap_entities_cached.cache_clear()
 
@@ -647,7 +647,7 @@ def five_entities_per_sitemap(mocker):
     from . import routes_browse
 
     mocker.patch.object(routes_browse, 'SITEMAP_MAX_URLS', 5)
-    mocker.patch('requests.post', side_effect=mock_many_entities_post(12))
+    mocker.patch('requests.Session.post', side_effect=mock_many_entities_post(12))
     routes_browse._get_sitemap_entities_cached.cache_clear()
     yield
     routes_browse._get_sitemap_entities_cached.cache_clear()