diff --git a/CHANGELOG.md b/CHANGELOG.md index 44e973794..617dc6c3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- `Connection` based requests: always use finite timeouts by default (20 minutes in general, 30 minutes for synchronous execute requests) + ([#454](https://github.com/Open-EO/openeo-python-client/issues/454)) + ### Removed ### Fixed diff --git a/openeo/rest/connection.py b/openeo/rest/connection.py index faf80862c..17765b042 100644 --- a/openeo/rest/connection.py +++ b/openeo/rest/connection.py @@ -51,6 +51,11 @@ _log = logging.getLogger(__name__) +# Default timeouts for requests +# TODO: get default_timeout from config? +DEFAULT_TIMEOUT = 20 * 60 +DEFAULT_TIMEOUT_SYNCHRONOUS_EXECUTE = 30 * 60 + class RestApiConnection: """Base connection class implementing generic REST API request functionality""" @@ -66,7 +71,7 @@ def __init__( self._root_url = root_url self.auth = auth or NullAuth() self.session = session or requests.Session() - self.default_timeout = default_timeout + self.default_timeout = default_timeout or DEFAULT_TIMEOUT self.default_headers = { "User-Agent": "openeo-python-client/{cv} {py}/{pv} {pl}".format( cv=openeo.client_version(), @@ -1388,7 +1393,7 @@ def download( self, graph: Union[dict, FlatGraphableMixin, str, Path], outputfile: Union[Path, str, None] = None, - timeout: int = 30 * 60, + timeout: Optional[int] = None, ) -> Union[None, bytes]: """ Downloads the result of a process graph synchronously, @@ -1401,7 +1406,13 @@ def download( :param timeout: timeout to wait for response """ request = self._build_request_with_process_graph(process_graph=graph) - response = self.post(path="/result", json=request, expected_status=200, stream=True, timeout=timeout) + response = self.post( + path="/result", + json=request, + expected_status=200, + stream=True, + timeout=timeout or DEFAULT_TIMEOUT_SYNCHRONOUS_EXECUTE, + ) if outputfile is not None: with Path(outputfile).open(mode="wb") as f: @@ -1410,7 +1421,11 @@ def download( else: return response.content - def execute(self, process_graph: Union[dict, str, Path]): + def execute( + self, + process_graph: Union[dict, str, Path], + timeout: Optional[int] = None, + ): """ Execute a process graph synchronously and return the result (assumed to be JSON). @@ -1419,7 +1434,12 @@ def execute(self, process_graph: Union[dict, str, Path]): :return: parsed JSON response """ req = self._build_request_with_process_graph(process_graph=process_graph) - return self.post(path="/result", json=req, expected_status=200).json() + return self.post( + path="/result", + json=req, + expected_status=200, + timeout=timeout or DEFAULT_TIMEOUT_SYNCHRONOUS_EXECUTE, + ).json() def create_job( self, diff --git a/tests/rest/test_connection.py b/tests/rest/test_connection.py index 942abb354..1b6a0393c 100644 --- a/tests/rest/test_connection.py +++ b/tests/rest/test_connection.py @@ -21,7 +21,14 @@ from openeo.rest.auth.auth import BearerAuth, NullAuth from openeo.rest.auth.oidc import OidcException from openeo.rest.auth.testing import ABSENT, OidcMock -from openeo.rest.connection import Connection, RestApiConnection, connect, paginate +from openeo.rest.connection import ( + Connection, + RestApiConnection, + connect, + paginate, + DEFAULT_TIMEOUT, + DEFAULT_TIMEOUT_SYNCHRONOUS_EXECUTE, +) from openeo.util import ContextTimer from .. import load_json_resource @@ -266,7 +273,7 @@ def test_connection_with_session(): conn = Connection("https://oeo.test/", session=session) assert conn.capabilities().capabilities["foo"] == "bar" session.request.assert_any_call( - url="https://oeo.test/", method="get", headers=mock.ANY, stream=mock.ANY, auth=mock.ANY, timeout=None + url="https://oeo.test/", method="get", headers=mock.ANY, stream=mock.ANY, auth=mock.ANY, timeout=DEFAULT_TIMEOUT ) @@ -278,7 +285,7 @@ def test_connect_with_session(): conn = connect("https://oeo.test/", session=session) assert conn.capabilities().capabilities["foo"] == "bar" session.request.assert_any_call( - url="https://oeo.test/", method="get", headers=mock.ANY, stream=mock.ANY, auth=mock.ANY, timeout=None + url="https://oeo.test/", method="get", headers=mock.ANY, stream=mock.ANY, auth=mock.ANY, timeout=DEFAULT_TIMEOUT ) @@ -2515,7 +2522,7 @@ def test_default_timeout_default(requests_mock): requests_mock.get(API_URL, json={"api_version": "1.0.0"}) requests_mock.get("/foo", text=lambda req, ctx: repr(req.timeout)) conn = connect(API_URL) - assert conn.get("/foo").text == 'None' + assert conn.get("/foo").text == str(DEFAULT_TIMEOUT) assert conn.get("/foo", timeout=5).text == '5' @@ -2532,6 +2539,32 @@ def flat_graph(self) -> typing.Dict[str, dict]: return {"foo1": {"process_id": "foo"}} +@pytest.mark.parametrize( + "pg", + [ + {"foo1": {"process_id": "foo"}}, + {"process_graph": {"foo1": {"process_id": "foo"}}}, + DummyFlatGraphable(), + ], +) +def test_download_100(requests_mock, pg): + requests_mock.get(API_URL, json={"api_version": "1.0.0"}) + conn = Connection(API_URL) + with mock.patch.object(conn, "request") as request: + conn.download(pg) + assert request.call_args_list == [ + mock.call( + "post", + path="/result", + allow_redirects=False, + stream=True, + expected_status=200, + json={"process": {"process_graph": {"foo1": {"process_id": "foo"}}}}, + timeout=DEFAULT_TIMEOUT_SYNCHRONOUS_EXECUTE, + ) + ] + + @pytest.mark.parametrize( "pg", [ @@ -2547,8 +2580,12 @@ def test_execute_100(requests_mock, pg): conn.execute(pg) assert request.call_args_list == [ mock.call( - "post", path="/result", allow_redirects=False, expected_status=200, - json={"process": {"process_graph": {"foo1": {"process_id": "foo"}}}} + "post", + path="/result", + allow_redirects=False, + expected_status=200, + json={"process": {"process_graph": {"foo1": {"process_id": "foo"}}}}, + timeout=DEFAULT_TIMEOUT_SYNCHRONOUS_EXECUTE, ) ]