ref(clients): Change defaults for timeouts (#246)

Our read timeouts have been causing issues on the server side since they
cause the connection to drop, which can leave operations in a dangling
state. This has to be addressed separately, but also uncovered that a
default read timeout is likely not desired by use cases.

This changes timeout functionality in both clients equally:

1.  There is a default connect timeout of 100ms
2.  There is no default read timeout anymore
3.  The `timeout` / `timeout_ms` setting only affects read timeouts
4.  To set connection timeouts, one has to use the lower-level client
    config.

This is a BREAKING CHANGE

Author: jan-auer

.vscode/settings.json

@@ -19,5 +19,8 @@
   "[markdown]": {
     "editor.rulers": [80],
     "editor.tabSize": 2,
+  },
+  "[python]": {
+    "editor.rulers": [88],
   }
 }

clients/python/docs/objectstore_client.rst

@@ -9,6 +9,14 @@ objectstore\_client package
 Submodules
 ----------
 
+objectstore\_client.auth module
+-------------------------------
+
+.. automodule:: objectstore_client.auth
+   :members:
+   :show-inheritance:
+   :undoc-members:
+
 objectstore\_client.client module
 ---------------------------------
 
@@ -32,3 +40,19 @@ objectstore\_client.metrics module
    :members:
    :show-inheritance:
    :undoc-members:
+
+objectstore\_client.scope module
+--------------------------------
+
+.. automodule:: objectstore_client.scope
+   :members:
+   :show-inheritance:
+   :undoc-members:
+
+objectstore\_client.utils module
+--------------------------------
+
+.. automodule:: objectstore_client.utils
+   :members:
+   :show-inheritance:
+   :undoc-members:

clients/python/pyproject.toml

@@ -3,11 +3,7 @@ name = "objectstore-client"
 version = "0.0.14"
 description = "Client SDK for Objectstore, the Sentry object storage platform"
 readme = "README.md"
-authors = [
-    {name = "Sentry", email = "[email protected]"},
-]
-homepage = "https://getsentry.github.io/objectstore/"
-repository = "https://github.com/getsentry/objectstore"
+authors = [{ name = "Sentry", email = "[email protected]" }]
 license = { file = "LICENSE.md" }
 requires-python = ">=3.11"
 dependencies = [

clients/python/src/objectstore_client/client.py

@@ -67,23 +67,61 @@ def __init__(
         self._expiration_policy = expiration_policy
 
 
+# Connect timeout used unless overridden in connection parameters.
+DEFAULT_CONNECT_TIMEOUT = 0.1
+
+
 @dataclass
 class _ConnectionDefaults:
     retries: urllib3.Retry = urllib3.Retry(connect=3, read=0)
     """We only retry connection problems, as we cannot rewind our compression stream."""
 
-    timeout: urllib3.Timeout = urllib3.Timeout(connect=0.5, read=0.5)
+    timeout: urllib3.Timeout = urllib3.Timeout(
+        connect=DEFAULT_CONNECT_TIMEOUT, read=None
+    )
     """
-    The read timeout is defined to be "between consecutive read operations",
-    which should mean one chunk of the response, with a large response being
-    split into multiple chunks.
-    We define both as 500ms which is still very conservative,
-    given that we are in the same network, and expect our backends to respond in <100ms.
+    The read timeout is defined to be "between consecutive read operations", which
+    should mean one chunk of the response, with a large response being split into
+    multiple chunks.
+
+    By default, the client limits the connection phase to 100ms, and has no read
+    timeout.
     """
 
 
 class Client:
-    """A client for Objectstore. Constructing it initializes a connection pool."""
+    """
+    A client for Objectstore. Constructing it initializes a connection pool.
+
+    Args:
+        base_url: The base URL of the Objectstore server (e.g.,
+            "http://objectstore:8888"). metrics_backend: Optional metrics backend for
+            tracking storage operations. Defaults to ``NoOpMetricsBackend`` if not
+            provided.
+        propagate_traces: Whether to propagate Sentry trace headers in requests to
+            objectstore. Defaults to ``False``.
+        retries: Number of connection retries for failed requests.
+            Defaults to ``3`` if not specified. **Note:** only connection failures are
+            retried, not read failures (as compression streams cannot be rewound).
+        timeout_ms: Read timeout in milliseconds for API requests. The read timeout
+            is the maximum time to wait between consecutive read operations on the
+            socket (i.e., between receiving chunks of data). Defaults to no read timeout
+            if not specified. The connection timeout is always 100ms. To override the
+            connection timeout, pass a custom ``urllib3.Timeout`` object via
+            ``connection_kwargs``. For example:
+
+            .. code-block:: python
+
+                client = Client(
+                    "http://objectstore:8888", connection_kwargs={
+                        "timeout": urllib3.Timeout(connect=1.0, read=5.0)
+                    }
+                )
+
+        connection_kwargs: Additional keyword arguments to pass to the underlying
+            urllib3 connection pool (e.g., custom headers, SSL settings, advanced
+            timeouts).
+    """
 
     def __init__(
         self,
@@ -107,7 +145,7 @@ def __init__(
 
         if timeout_ms:
             connection_kwargs_to_use["timeout"] = urllib3.Timeout(
-                connect=timeout_ms / 1000, read=timeout_ms / 1000
+                connect=DEFAULT_CONNECT_TIMEOUT, read=timeout_ms / 1000
             )
 
         if connection_kwargs:

clients/python/tests/test_e2e.py

@@ -231,14 +231,36 @@ def test_fails_with_insufficient_auth_perms(server_url: str) -> None:
         _object_key = session.put(b"test data")
 
 
-def test_connect_timeout() -> None:
+def test_read_timeout() -> None:
     # this server accepts the connection
     # (even though the backlog is 0 and we never call `accept`),
     # but will never reply with anything, thus causing a read timeout
     s = socket.create_server(("127.0.0.1", 0), backlog=0)
     addr = s.getsockname()
     url = f"http://127.0.0.1:{addr[1]}"
 
+    client = Client(url, timeout_ms=500, token_generator=TestTokenGenerator.get())
+    test_usecase = Usecase(
+        "test-usecase",
+        compression="zstd",
+        expiration_policy=TimeToLive(timedelta(days=1)),
+    )
+
+    session = client.session(
+        test_usecase, org=12345, project=1337, app_slug="email_app"
+    )
+
+    with pytest.raises(urllib3.exceptions.MaxRetryError):
+        session.get("foo")
+
+
+def test_connect_timeout() -> None:
+    # Connect to a blackhole address that should not reply to SYN packets,
+    # causing a connect timeout with the client's default connect timeout.
+    # 10.255.255.1 is commonly unroutable in most environments.
+    url = "http://10.255.255.1:9"
+
+    # Do NOT set timeout_ms to ensure we exercise default timeouts
     client = Client(url, token_generator=TestTokenGenerator.get())
     test_usecase = Usecase(
         "test-usecase",

clients/rust/src/client.rs

@@ -56,13 +56,10 @@ impl ClientBuilder {
         }
 
         let reqwest_builder = reqwest::Client::builder()
-            // The read timeout "applies to each read operation", so should work fine for larger
-            // transfers that are split into multiple chunks.
-            // We define both as 500ms which is still very conservative, given that we are in the same network,
-            // and expect our backends to respond in <100ms.
-            // This can be overridden by the caller.
-            .connect_timeout(Duration::from_millis(500))
-            .read_timeout(Duration::from_millis(500))
+            // We define just a connection timeout by default but do not limit reads. A connect
+            // timeout of 100ms is still very conservative, but should provide a sensible upper
+            // bound to expected request latencies.
+            .connect_timeout(Duration::from_millis(100))
             .user_agent(USER_AGENT);
 
         Self(Ok(ClientBuilderInner {
@@ -84,16 +81,16 @@ impl ClientBuilder {
         self
     }
 
-    /// Sets both the connect and the read timeout for the [`reqwest::Client`].
-    /// For more fine-grained configuration, use [`Self::configure_reqwest`].
+    /// Defines a read timeout for the [`reqwest::Client`].
     ///
-    /// By default, a connect and read timeout of 500ms is set.
+    /// The read timeout is defined to be "between consecutive read operations", for example between
+    /// chunks of a streaming response. For more fine-grained configuration of this and other
+    /// timeouts, use [`Self::configure_reqwest`].
+    ///
+    /// By default, no read timeout and a connect timeout of 100ms is set.
     pub fn timeout(self, timeout: Duration) -> Self {
         let Ok(mut inner) = self.0 else { return self };
-        inner.reqwest_builder = inner
-            .reqwest_builder
-            .connect_timeout(timeout)
-            .read_timeout(timeout);
+        inner.reqwest_builder = inner.reqwest_builder.read_timeout(timeout);
         Self(Ok(inner))
     }