raise exceptions on non-200 HTTP returns and retry implicitly

Requests does not throw on HTTP errors, so pyosmium was just consuming
the error message. Fix that. Also add a retry mechanism for
transient errors. The list of transient status codes is the
same as the one that curl uses.

Author: lonvia

src/osmium/replication/server.py

@@ -7,14 +7,17 @@
 """ Helper functions to communicate with replication servers.
 """
 from typing import NamedTuple, Optional, Any, Iterator, cast, Dict, Mapping, Tuple
-import requests
 import urllib.request as urlrequest
 from urllib.error import URLError
 import datetime as dt
 from collections import namedtuple
 from contextlib import contextmanager
 from math import ceil
 
+import requests
+from requests.adapters import HTTPAdapter
+from urllib3.util import Retry
+
 from osmium import MergeInputReader, BaseHandler
 from osmium import io as oio
 from osmium import version
@@ -52,6 +55,8 @@ def __init__(self, url: str, diff_type: str = 'osc.gz') -> None:
         self.diff_type = diff_type
         self.extra_request_params: dict[str, Any] = dict(timeout=60, stream=True)
         self.session: Optional[requests.Session] = None
+        self.retry = Retry(total=3, backoff_factor=0.5, allowed_methods={'GET'},
+                           status_forcelist=[408, 429, 500, 502, 503, 504])
 
     def close(self) -> None:
         """ Close any open connection to the replication server.
@@ -62,6 +67,8 @@ def close(self) -> None:
 
     def __enter__(self) -> 'ReplicationServer':
         self.session = requests.Session()
+        self.session.mount('http://', HTTPAdapter(max_retries=self.retry))
+        self.session.mount('https://', HTTPAdapter(max_retries=self.retry))
         return self
 
     def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None:
@@ -97,6 +104,8 @@ def open_url(self, url: urlrequest.Request) -> Any:
         @contextmanager
         def _get_url_with_session() -> Iterator[requests.Response]:
             with requests.Session() as session:
+                session.mount('http://', HTTPAdapter(max_retries=self.retry))
+                session.mount('https://', HTTPAdapter(max_retries=self.retry))
                 request = session.get(url.get_full_url(), **get_params)
                 yield request
 
@@ -133,7 +142,7 @@ def collect_diffs(self, start_id: int, max_size: int = 1024) -> Optional[Downloa
             try:
                 diffdata = self.get_diff_block(current_id)
             except:
-                LOG.debug("Error during diff download. Bailing out.")
+                LOG.error("Error during diff download. Bailing out.")
                 diffdata = ''
             if len(diffdata) == 0:
                 if start_id == current_id:
@@ -348,6 +357,7 @@ def get_state_info(self, seq: Optional[int] = None, retries: int = 2) -> Optiona
                 with self.open_url(self.make_request(self.get_state_url(seq))) as response:
                     if hasattr(response, 'iter_lines'):
                         # generated by requests
+                        response.raise_for_status()
                         lines = response.iter_lines()
                     else:
                         lines = response
@@ -372,7 +382,7 @@ def get_state_info(self, seq: Optional[int] = None, retries: int = 2) -> Optiona
                                 ts = ts.replace(tzinfo=dt.timezone.utc)
 
             except (URLError, IOError) as err:
-                LOG.debug("Loading state info %s failed with: %s", seq, str(err))
+                LOG.debug("Loading state info failed with: %s", str(err))
                 return None
 
             if ts is not None and next_seq is not None:
@@ -382,12 +392,13 @@ def get_state_info(self, seq: Optional[int] = None, retries: int = 2) -> Optiona
 
     def get_diff_block(self, seq: int) -> str:
         """ Downloads the diff with the given sequence number and returns
-            it as a byte sequence. Throws a :code:`urllib.error.HTTPError`
+            it as a byte sequence. Throws an :code:`requests.HTTPError`
             if the file cannot be downloaded.
         """
         with self.open_url(self.make_request(self.get_diff_url(seq))) as resp:
             if hasattr(resp, 'content'):
                 # generated by requests
+                resp.raise_for_status()
                 return cast(str, resp.content)
 
             # generated by urllib.request

test/test_replication.py

@@ -3,6 +3,7 @@
 # This file is part of Pyosmium.
 #
 # Copyright (C) 2023 Sarah Hoffmann.
+import logging
 import time
 from textwrap import dedent
 
@@ -124,6 +125,35 @@ def test_get_state_timestamp_cut(httpserver):
     assert res.sequence == 2594669
 
 
+def test_get_state_permanent_error(httpserver, caplog):
+    httpserver.expect_request('/state.txt').respond_with_data('stuff', status=404)
+
+    with caplog.at_level(logging.DEBUG):
+        res = rserv.ReplicationServer(httpserver.url_for('')).get_state_info()
+
+    assert res is None
+    assert "Loading state info failed" in caplog.text
+
+
+def test_get_state_transient_error(httpserver):
+    httpserver.expect_ordered_request('/state.txt').respond_with_data('stuff', status=500)
+    httpserver.expect_ordered_request('/state.txt').respond_with_data('stuff', status=500)
+    httpserver.expect_ordered_request('/state.txt').respond_with_data("""\
+        #Sat Aug 26 11:04:04 UTC 2017
+        txnMaxQueried=1219304113
+        sequenceNumber=2594669
+        timestamp=2017-08-26T11\\:04\\:02Z
+        txnReadyList=
+        txnMax=1219304113
+        txnActiveList=1219303583,1219304054,1219304104""")
+
+    res = rserv.ReplicationServer(httpserver.url_for('')).get_state_info()
+
+    assert res is not None
+    assert res.timestamp == mkdate(2017, 8, 26, 11, 4, 2)
+    assert res.sequence == 2594669
+
+
 def test_get_state_too_many_retries(httpserver):
     httpserver.expect_ordered_request('/state.txt').respond_with_data("""\
         #Sat Aug 26 11:04:04 UTC 2017
@@ -321,3 +351,92 @@ def way(self, w):
     assert diffs is not None
     diffs.reader.apply(h, idx="flex_mem")
     assert h.counts == [1, 1, 0, 0]
+
+
+def test_apply_diffs_permanent_error(httpserver, caplog):
+    httpserver.expect_ordered_request('/state.txt').respond_with_data("""\
+        sequenceNumber=100
+        timestamp=2017-08-26T11\\:04\\:02Z
+    """)
+    httpserver.expect_ordered_request('/000/000/100.opl')\
+              .respond_with_data('not a file', status=404)
+
+    with caplog.at_level(logging.ERROR):
+        with rserv.ReplicationServer(httpserver.url_for(''), "opl") as svr:
+            h = CountingHandler()
+            assert None == svr.apply_diffs(h, 100, 10000)
+            assert h.counts == [0, 0, 0, 0]
+
+    assert 'Error during diff download' in caplog.text
+
+
+def test_apply_diffs_permanent_error_later_diff(httpserver, caplog):
+    httpserver.expect_ordered_request('/state.txt').respond_with_data("""\
+        sequenceNumber=101
+        timestamp=2017-08-26T11\\:04\\:02Z
+    """)
+    httpserver.expect_ordered_request('/000/000/100.opl').respond_with_data(dedent("""\
+        n1 x10.0 y23.0
+        w1 Nn1,n2
+    """))
+    httpserver.expect_ordered_request('/000/000/101.opl')\
+              .respond_with_data('not a file', status=404)
+
+    with caplog.at_level(logging.ERROR):
+        with rserv.ReplicationServer(httpserver.url_for(''), "opl") as svr:
+            h = CountingHandler()
+            assert 100 == svr.apply_diffs(h, 100, 10000)
+            assert h.counts == [1, 1, 0, 0]
+
+    assert 'Error during diff download' in caplog.text
+
+
+def test_apply_diffs_transient_error(httpserver, caplog):
+    httpserver.expect_ordered_request('/state.txt').respond_with_data("""\
+        sequenceNumber=101
+        timestamp=2017-08-26T11\\:04\\:02Z
+    """)
+    httpserver.expect_ordered_request('/000/000/100.opl').respond_with_data(dedent("""\
+        n1 x10.0 y23.0
+        w1 Nn1,n2
+    """))
+    httpserver.expect_ordered_request('/000/000/101.opl')\
+              .respond_with_data('not a file', status=503)
+    httpserver.expect_ordered_request('/000/000/101.opl').respond_with_data(dedent("""\
+        n2 x10.0 y23.0
+    """))
+
+    with caplog.at_level(logging.ERROR):
+        with rserv.ReplicationServer(httpserver.url_for(''), "opl") as svr:
+            h = CountingHandler()
+            assert 101 == svr.apply_diffs(h, 100, 10000)
+            assert h.counts == [2, 1, 0, 0]
+
+    assert 'Error during diff download' not in caplog.text
+
+
+
+
+def test_apply_diffs_transient_error_permanent(httpserver, caplog):
+    httpserver.expect_ordered_request('/state.txt').respond_with_data("""\
+        sequenceNumber=101
+        timestamp=2017-08-26T11\\:04\\:02Z
+    """)
+    httpserver.expect_ordered_request('/000/000/100.opl').respond_with_data(dedent("""\
+        n1 x10.0 y23.0
+        w1 Nn1,n2
+    """))
+    for _ in range(4):
+        httpserver.expect_ordered_request('/000/000/101.opl')\
+                  .respond_with_data('not a file', status=503)
+    httpserver.expect_ordered_request('/000/000/101.opl').respond_with_data(dedent("""\
+        n2 x10.0 y23.0
+    """))
+
+    with caplog.at_level(logging.ERROR):
+        with rserv.ReplicationServer(httpserver.url_for(''), "opl") as svr:
+            h = CountingHandler()
+            assert 100 == svr.apply_diffs(h, 100, 10000)
+            assert h.counts == [1, 1, 0, 0]
+
+    assert 'Error during diff download' in caplog.text