timeouts for http clients (#1918)

* trying 1s timeout for webservr proxy

* adding timeout for the rest client

* adding timeout for the file manager downloads

* adding timeouts for httpx connections

* removed autoformatting

* added missing dependency

* bounded clientsessin requests @sanderegg please check

* services are expected to respond in 10 seconds max

* bumping timeouts to 20 seconds up from 10

* added timeout for requests in servicelib

* bumping to 5 seconds for good measure

* bumping to 5 seconds for good measure

* timeout moved to enviornment var

* timeout moved to enviornment variable

* migrated settings to enviornment

* fixed broken test

* fixed broken test

* fixed issue due to missleading naming

* fixed type

* moving ClientRequestSettings to models-library

* settings are recovered via app instance

* bumped to 5 seconds from 1

* refactoring arguments @sanderegg

* loading form env vars

* added changes to autogen code

* loading for env via pydantic

* added missing commits

* updated import path

* corrected wrong path

* added test to make sure timeouts are in palce

* removing beacause it is already in models_library

* refactored to use fixture for current directory path

* refactored the test
- provided a more helpful error
- checks for the timeout=timeout parameter

Co-authored-by: Andrei Neagu <[email protected]>

Author: GitHK

packages/models-library/src/models_library/settings/http_clients.py

@@ -0,0 +1,32 @@
+from typing import Optional
+
+from pydantic import BaseSettings, Field
+
+
+class ClientRequestSettings(BaseSettings):
+    # NOTE: when updating the defaults please make sure to search for the env vars
+    # in all the project, they also need to be updated inside the service-library
+    total_timeout: Optional[int] = Field(
+        default=20,
+        description="timeout used for outgoing http requests",
+        env="HTTP_CLIENT_REQUEST_TOTAL_TIMEOUT",
+    )
+
+    aiohttp_connect_timeout: Optional[int] = Field(
+        default=5,
+        description=(
+            "aiohttp specific field used in ClientTimeout, timeout for connecting to a "
+            "peer for a new connection waiting for a free connection from a pool if "
+            "pool connection limits are exceeded"
+        ),
+        env="HTTP_CLIENT_REQUEST_AIOHTTP_CONNECT_TIMEOUT",
+    )
+
+    aiohttp_sock_connect_timeout: Optional[int] = Field(
+        default=5,
+        description=(
+            "aiohttp specific field used in ClientTimeout, timeout for connecting to a "
+            "peer for a new connection not given a pool"
+        ),
+        env="HTTP_CLIENT_REQUEST_AIOHTTP_SOCK_CONNECT_TIMEOUT",
+    )

packages/service-library/src/servicelib/client_session.py

@@ -5,26 +5,40 @@
 """
 import logging
 
-from aiohttp import ClientSession, web
+from aiohttp import ClientSession, ClientTimeout, web
 
 from .application_keys import APP_CLIENT_SESSION_KEY
+from .utils import (
+    get_http_client_request_total_timeout,
+    get_http_client_request_aiohttp_connect_timeout,
+    get_http_client_request_aiohttp_sock_connect_timeout,
+)
 
 log = logging.getLogger(__name__)
 
 
 def get_client_session(app: web.Application) -> ClientSession:
-    """ Lazy initialization of ClientSession
+    """Lazy initialization of ClientSession
 
     Ensures unique session
     """
     session = app.get(APP_CLIENT_SESSION_KEY)
     if session is None or session.closed:
-        app[APP_CLIENT_SESSION_KEY] = session = ClientSession()
+        # it is important to have fast connection handshakes
+        # also requests should be as fast as possible
+        # some services are not that fast to  reply
+        # Setting the time of a request using this client session to 5 seconds totals
+        timeout_settings = ClientTimeout(
+            total=get_http_client_request_total_timeout(),
+            connect=get_http_client_request_aiohttp_connect_timeout(),
+            sock_connect=get_http_client_request_aiohttp_sock_connect_timeout(),
+        )
+        app[APP_CLIENT_SESSION_KEY] = session = ClientSession(timeout=timeout_settings)
     return session
 
 
 async def persistent_client_session(app: web.Application):
-    """ Ensures a single client session per application
+    """Ensures a single client session per application
 
     IMPORTANT: Use this function ONLY in cleanup context , i.e.
         app.cleanup_ctx.append(persistent_client_session)

packages/service-library/src/servicelib/utils.py

@@ -15,14 +15,26 @@
 
 def is_production_environ() -> bool:
     """
-        If True, this code most probably
-        runs in a production container of one of the
-        osparc-simcore services.
+    If True, this code most probably
+    runs in a production container of one of the
+    osparc-simcore services.
     """
     # WARNING: based on a convention that is not constantly verified
     return os.environ.get("SC_BUILD_TARGET") == "production"
 
 
+def get_http_client_request_total_timeout() -> int:
+    return int(os.environ.get("HTTP_CLIENT_REQUEST_TOTAL_TIMEOUT", "20"))
+
+
+def get_http_client_request_aiohttp_connect_timeout() -> int:
+    return int(os.environ.get("HTTP_CLIENT_REQUEST_AIOHTTP_CONNECT_TIMEOUT", "5"))
+
+
+def get_http_client_request_aiohttp_sock_connect_timeout() -> int:
+    return int(os.environ.get("HTTP_CLIENT_REQUEST_AIOHTTP_SOCK_CONNECT_TIMEOUT", "5"))
+
+
 def is_osparc_repo_dir(path: Path) -> bool:
     # TODO: implement with git cli
     expected = (".github", "packages", "services")
@@ -31,9 +43,9 @@ def is_osparc_repo_dir(path: Path) -> bool:
 
 
 def search_osparc_repo_dir(start: Union[str, Path], max_iterations=8) -> Optional[Path]:
-    """ Returns path to root repo dir or None if it does not exists
+    """Returns path to root repo dir or None if it does not exists
 
-        NOTE: assumes starts is a path within repo
+    NOTE: assumes starts is a path within repo
     """
     max_iterations = max(max_iterations, 1)
     root_dir = Path(start)

packages/simcore-sdk/src/simcore_sdk/config/http_clients.py

@@ -0,0 +1,4 @@
+from models_library.settings.http_clients import ClientRequestSettings
+
+
+client_request_settings = ClientRequestSettings()

packages/simcore-sdk/src/simcore_sdk/node_ports/filemanager.py

@@ -5,7 +5,7 @@
 from pathlib import Path
 from typing import Optional
 
-from aiohttp import ClientSession
+from aiohttp import ClientSession, ClientTimeout
 from yarl import URL
 
 import aiofiles
@@ -14,6 +14,7 @@
 from models_library.settings.services_common import ServicesCommonSettings
 
 from . import config, exceptions
+from ..config.http_clients import client_request_settings
 
 log = logging.getLogger(__name__)
 
@@ -28,7 +29,16 @@ class ClientSessionContextManager:
     # See https://github.com/ITISFoundation/osparc-simcore/issues/1098
     #
     def __init__(self, session=None):
-        self.active_session = session or ClientSession()
+        # We are interested in fast connections, if a connection is established
+        # there is no timeout for file download operations
+
+        self.active_session = session or ClientSession(
+            timeout=ClientTimeout(
+                total=None,
+                connect=client_request_settings.aiohttp_connect_timeout,
+                sock_connect=client_request_settings.aiohttp_sock_connect_timeout,
+            )
+        )
         self.is_owned = self.active_session is not session
 
     async def __aenter__(self):

services/api-server/requirements/_base.in

@@ -3,6 +3,7 @@
 #
 # NOTE: ALL version constraints MUST be commented
 -c ../../../requirements/constraints.txt
+-r ../../../packages/models-library/requirements/_base.in
 -r ../../../packages/postgres-database/requirements/_base.in
 
 # fastapi and extensions

services/api-server/requirements/ci.txt

@@ -11,6 +11,7 @@
 -r _test.txt
 
 # installs this repo's packages
+../../packages/models-library
 ../../packages/pytest-simcore/
 ../../packages/postgres-database/
 

services/api-server/requirements/dev.txt

@@ -12,6 +12,7 @@
 -r _tools.txt
 
 # installs this repo's packages
+-e ../../packages/models-library
 -e ../../packages/pytest-simcore/
 -e ../../packages/postgres-database/[migration]
 

services/api-server/requirements/prod.txt

@@ -10,6 +10,7 @@
 -r _base.txt
 
 # installs this repo's packages
+../../packages/models-library
 ../../packages/postgres-database/
 
 # installs current package

services/api-server/setup.py

@@ -24,7 +24,9 @@ def read_reqs(reqs_path: Path):
 readme = (current_dir / "README.md").read_text()
 version = (current_dir / "VERSION").read_text().strip()
 
-install_requirements = read_reqs(current_dir / "requirements" / "_base.txt")
+install_requirements = read_reqs(current_dir / "requirements" / "_base.txt") + [
+    "simcore-models-library"
+]
 
 test_requirements = read_reqs(current_dir / "requirements" / "_test.txt")
 

services/api-server/src/simcore_service_api_server/core/settings.py

@@ -5,6 +5,8 @@
 from pydantic import BaseSettings, Field, SecretStr, validator
 from yarl import URL
 
+from models_library.settings.http_clients import ClientRequestSettings
+
 
 class BootModeEnum(str, Enum):
     debug = "debug-ptvsd"
@@ -67,7 +69,11 @@ class AppSettings(BaseSettings):
     @classmethod
     def create_default(cls) -> "AppSettings":
         # This call triggers parsers
-        return cls(postgres=PostgresSettings(), webserver=WebServerSettings())
+        return cls(
+            postgres=PostgresSettings(),
+            webserver=WebServerSettings(),
+            client_request=ClientRequestSettings(),
+        )
 
     # pylint: disable=no-self-use
     # pylint: disable=no-self-argument
@@ -86,7 +92,6 @@ def match_logging_level(cls, value) -> str:
         except AttributeError as err:
             raise ValueError(f"{value.upper()} is not a valid level") from err
 
-
     @property
     def loglevel(self) -> int:
         return getattr(logging, self.log_level_name)
@@ -97,6 +102,8 @@ def loglevel(self) -> int:
     # WEB-SERVER SERVICE
     webserver: WebServerSettings
 
+    client_request: ClientRequestSettings
+
     # SERVICE SERVER (see : https://www.uvicorn.org/settings/)
     host: str = "0.0.0.0"  # nosec
     port: int = 8000

services/api-server/src/simcore_service_api_server/services/webserver.py

@@ -38,7 +38,10 @@ def setup_webserver(app: FastAPI) -> None:
     # init client
     logger.debug("Setup webserver at %s...", settings.base_url)
 
-    client = AsyncClient(base_url=settings.base_url)
+    client = AsyncClient(
+        base_url=settings.base_url,
+        timeout=app.state.settings.client_request.total_timeout,
+    )
     app.state.webserver_client = client
 
     # TODO: raise if attribute already exists
@@ -108,7 +111,7 @@ def _process(cls, resp: Response) -> Optional[Dict]:
     # TODO: add ping to healthcheck
 
     async def get(self, path: str) -> Optional[Dict]:
-        url = path.lstrip('/')
+        url = path.lstrip("/")
         try:
             resp = await self.client.get(url, cookies=self.session_cookies)
         except Exception as err:
@@ -118,7 +121,7 @@ async def get(self, path: str) -> Optional[Dict]:
         return self._process(resp)
 
     async def put(self, path: str, body: Dict) -> Optional[Dict]:
-        url = path.lstrip('/')
+        url = path.lstrip("/")
         try:
             resp = await self.client.put(url, json=body, cookies=self.session_cookies)
         except Exception as err:

services/api-server/tests/unit/test_settings.py

@@ -8,6 +8,7 @@
     BootModeEnum,
     PostgresSettings,
     WebServerSettings,
+    ClientRequestSettings,
 )
 
 
@@ -26,7 +27,11 @@ def test_min_environ_for_settings(monkeypatch):
     monkeypatch.setenv("SC_BOOT_MODE", "production")
 
     # NOTE: pg and weberver settings parse environ NOW!
-    settings = AppSettings(postgres=PostgresSettings(), webserver=WebServerSettings())
+    settings = AppSettings(
+        postgres=PostgresSettings(),
+        webserver=WebServerSettings(),
+        client_request=ClientRequestSettings(),
+    )
 
     pprint(settings.dict())
 

services/catalog/src/simcore_service_catalog/core/settings.py

@@ -6,6 +6,8 @@
 from pydantic.types import PositiveInt
 from yarl import URL
 
+from models_library.settings.http_clients import ClientRequestSettings
+
 
 class BootModeEnum(str, Enum):
     DEBUG = "debug-ptvsd"
@@ -66,7 +68,11 @@ class AppSettings(BaseSettings):
     @classmethod
     def create_default(cls) -> "AppSettings":
         # This call triggers parsers
-        return cls(postgres=PostgresSettings(), director=DirectorSettings())
+        return cls(
+            postgres=PostgresSettings(),
+            director=DirectorSettings(),
+            client_request=ClientRequestSettings(),
+        )
 
     # pylint: disable=no-self-use
     # pylint: disable=no-self-argument
@@ -92,6 +98,8 @@ def loglevel(self) -> int:
     # POSTGRES
     postgres: PostgresSettings
 
+    client_request: ClientRequestSettings
+
     # DIRECTOR SERVICE
     director: DirectorSettings
 

services/catalog/src/simcore_service_catalog/services/director.py

@@ -17,9 +17,7 @@ def setup_director(app: FastAPI) -> None:
 
     # init client-api
     logger.debug("Setup director at %s...", settings.base_url)
-    app.state.director_api = DirectorApi(
-        base_url=settings.base_url, vtag=app.state.settings.director.vtag
-    )
+    app.state.director_api = DirectorApi(base_url=settings.base_url, app=app)
 
     # does NOT communicate with director service
 
@@ -97,9 +95,11 @@ class DirectorApi:
     SEE services/catalog/src/simcore_service_catalog/api/dependencies/director.py
     """
 
-    def __init__(self, base_url: str, vtag: str):
-        self.client = AsyncClient(base_url=base_url)
-        self.vtag = vtag
+    def __init__(self, base_url: str, app: FastAPI):
+        self.client = AsyncClient(
+            base_url=base_url, timeout=app.state.settings.client_request.total_timeout
+        )
+        self.vtag = app.state.settings.director.vtag
 
     # OPERATIONS
     # TODO: policy to retry if NetworkError/timeout?

services/catalog/tests/unit/test_services_director.py

@@ -13,6 +13,7 @@
     AppSettings,
     DirectorSettings,
     PostgresSettings,
+    ClientRequestSettings,
 )
 from simcore_service_catalog.services.director import DirectorApi
 
@@ -25,6 +26,7 @@ def minimal_app(loop, devel_environ) -> FastAPI:
     settings = AppSettings(
         postgres=PostgresSettings(enabled=False),
         director=DirectorSettings(enabled=False),
+        client_request=ClientRequestSettings(),
     )
     app = init_app(settings)
 

services/director-v2/src/simcore_service_director_v2/core/settings.py

@@ -6,6 +6,7 @@
 from models_library.basic_types import BootModeEnum, PortInt
 from models_library.settings.celery import CeleryConfig
 from models_library.settings.postgres import PostgresSettings
+from models_library.settings.http_clients import ClientRequestSettings
 from pydantic import (
     BaseSettings,
     Field,
@@ -127,6 +128,7 @@ def create_from_env(cls, **settings_kwargs) -> "AppSettings":
             registry=RegistrySettings(),
             celery=CelerySettings.create_from_env(),
             dynamic_services=DynamicServicesSettings(),
+            client_request=ClientRequestSettings(),
             **settings_kwargs,
         )
 
@@ -223,5 +225,7 @@ def loglevel(self) -> int:
 
     remote_debug_port: PortInt = 3000
 
+    client_request: ClientRequestSettings
+
     class Config(CommonConfig):
         env_prefix = ""