diff --git a/docs/src/js/interfaces/TimeoutConfig.md b/docs/src/js/interfaces/TimeoutConfig.md index c821afc4d..74e174522 100644 --- a/docs/src/js/interfaces/TimeoutConfig.md +++ b/docs/src/js/interfaces/TimeoutConfig.md @@ -45,6 +45,10 @@ The timeout for reading data from the server in seconds. Default is 300 seconds (5 minutes). This can also be set via the environment variable `LANCE_CLIENT_READ_TIMEOUT`, as an integer number of seconds. +This also applies while waiting for response headers during an upload. +A large or slowly produced request body (for example, when creating a +table) may need a longer timeout even if data is still being uploaded. + *** ### timeout? diff --git a/nodejs/src/remote.rs b/nodejs/src/remote.rs index d7e2a21cd..c776ea597 100644 --- a/nodejs/src/remote.rs +++ b/nodejs/src/remote.rs @@ -25,6 +25,10 @@ pub struct TimeoutConfig { /// The timeout for reading data from the server in seconds. Default is 300 /// seconds (5 minutes). This can also be set via the environment variable /// `LANCE_CLIENT_READ_TIMEOUT`, as an integer number of seconds. + /// + /// This also applies while waiting for response headers during an upload. + /// A large or slowly produced request body (for example, when creating a + /// table) may need a longer timeout even if data is still being uploaded. pub read_timeout: Option, /// The timeout for keeping idle connections in the connection pool in seconds. /// Default is 300 seconds (5 minutes). This can also be set via the diff --git a/python/python/lancedb/remote/__init__.py b/python/python/lancedb/remote/__init__.py index a1c8ebee5..bb9a32122 100644 --- a/python/python/lancedb/remote/__init__.py +++ b/python/python/lancedb/remote/__init__.py @@ -52,7 +52,10 @@ class TimeoutConfig: `LANCE_CLIENT_CONNECT_TIMEOUT`, as an integer number of seconds. read_timeout: Optional[timedelta] The timeout for reading data from the server. Default is 300 seconds - (5 minutes). This can also be set via the environment variable + (5 minutes). This also applies while waiting for response headers during + an upload. Increase it for large or slowly produced inputs to + `create_table`, even if data is still being uploaded. This can also be + set via the environment variable `LANCE_CLIENT_READ_TIMEOUT`, as an integer number of seconds. pool_idle_timeout: Optional[timedelta] The timeout for keeping idle connections in the connection pool. Default diff --git a/python/python/tests/test_remote_db.py b/python/python/tests/test_remote_db.py index 32f999d18..b2515cd26 100644 --- a/python/python/tests/test_remote_db.py +++ b/python/python/tests/test_remote_db.py @@ -811,6 +811,43 @@ def test_create_table_exist_ok_with_mode_overwrite(): assert table is not None +@pytest.mark.asyncio +@pytest.mark.parametrize( + "timeout_config", + [{"read_timeout": 0.5}, {"timeout": 0.5, "read_timeout": 5}], +) +async def test_create_table_timeout_is_actionable(timeout_config): + release_response = threading.Event() + received = {} + + def handler(request): + received["path"] = request.path + received["request_id"] = request.headers["x-request-id"] + release_response.wait(timeout=5) + request.close_connection = True + + async with mock_lancedb_connection_async( + handler, timeout_config=timeout_config + ) as db: + batch = pa.record_batch({"id": [1, 2]}) + reader = pa.RecordBatchReader.from_batches(batch.schema, [batch]) + try: + with pytest.raises(HttpError) as exc_info: + await db.create_table("test", reader) + finally: + release_response.set() + + error = exc_info.value + assert "timed out" in str(error) + assert "client_config.timeout_config.read_timeout" in str(error) + assert "client_config.timeout_config.timeout" in str(error) + assert "LANCE_CLIENT_READ_TIMEOUT" in str(error) + assert received["path"] == "/v1/table/test/create/?mode=create" + assert error.request_id == received["request_id"] + assert error.status_code is None + assert isinstance(error.__cause__, HttpError) + + @pytest.mark.asyncio async def test_http_error(): request_id_holder = {"request_id": None} diff --git a/rust/lancedb/src/error.rs b/rust/lancedb/src/error.rs index 6052b67d8..1b1f5b0a5 100644 --- a/rust/lancedb/src/error.rs +++ b/rust/lancedb/src/error.rs @@ -10,6 +10,25 @@ use snafu::Snafu; pub(crate) type BoxError = Box; +// reqwest's Display omits the timeout cause. Add guidance at display time so +// callers still receive the original typed source and its full error chain. +#[cfg(feature = "remote")] +fn http_timeout_hint(source: &(dyn std::error::Error + 'static)) -> &'static str { + match source.downcast_ref::() { + Some(error) if error.is_timeout() && error.is_connect() => { + "; connection timed out. Check client_config.timeout_config.connect_timeout \ + (LANCE_CLIENT_CONNECT_TIMEOUT) and client_config.timeout_config.timeout \ + (LANCE_CLIENT_TIMEOUT) if configured" + } + Some(error) if error.is_timeout() => { + "; request timed out. For slow uploads or responses, check \ + client_config.timeout_config.read_timeout (LANCE_CLIENT_READ_TIMEOUT) \ + and client_config.timeout_config.timeout (LANCE_CLIENT_TIMEOUT) if configured" + } + _ => "", + } +} + /// Why a job failed, to whatever precision the backend provides. /// /// A job run in this process carries the error it failed with in [`Self::source`]. @@ -111,7 +130,10 @@ pub enum Error { #[snafu(display("lance error: {source}"))] Lance { source: lance::Error }, #[cfg(feature = "remote")] - #[snafu(display("Http error: (request_id={request_id}) {source}"))] + #[snafu(display( + "Http error: (request_id={request_id}) {source}{}", + http_timeout_hint(source.as_ref()) + ))] Http { #[snafu(source(from(reqwest::Error, Box::new)))] source: Box, @@ -127,7 +149,8 @@ pub enum Error { "Hit retry limit for request_id={request_id} (\ request_failures={request_failures}/{max_request_failures}, \ connect_failures={connect_failures}/{max_connect_failures}, \ - read_failures={read_failures}/{max_read_failures}): {source}" + read_failures={read_failures}/{max_read_failures}): {source}{}", + http_timeout_hint(source.as_ref()) ))] Retry { request_id: String, @@ -314,6 +337,20 @@ impl From for Error { mod tests { use super::*; + #[cfg(feature = "remote")] + #[test] + fn non_timeout_http_error_message_is_unchanged() { + let error = Error::Http { + source: "server error".into(), + request_id: "test".into(), + status_code: Some(reqwest::StatusCode::INTERNAL_SERVER_ERROR), + }; + assert_eq!( + error.to_string(), + "Http error: (request_id=test) server error" + ); + } + #[test] fn unsupported_filesystem_operations_have_actionable_error() { let object_store_error = object_store::Error::Generic { diff --git a/rust/lancedb/src/remote/client.rs b/rust/lancedb/src/remote/client.rs index f54e2fe3e..bf90b1e77 100644 --- a/rust/lancedb/src/remote/client.rs +++ b/rust/lancedb/src/remote/client.rs @@ -233,6 +233,10 @@ pub struct TimeoutConfig { pub connect_timeout: Option, /// The timeout for reading a response from the server. /// + /// This also applies while waiting for response headers during an upload. + /// A large or slowly produced request body (for example, when creating a + /// table) may need a longer timeout even if data is still being uploaded. + /// /// You can also set the `LANCE_CLIENT_READ_TIMEOUT` environment variable /// to set this value. Use an integer value in seconds. /// @@ -1205,6 +1209,117 @@ pub mod test_utils { #[cfg(test)] mod tests { + #[tokio::test] + async fn test_http_timeout_error_is_actionable() { + use super::*; + use futures::StreamExt; + + // Keep consuming the upload without sending response headers (HTTP) or + // completing the TLS handshake (HTTPS). The HTTP body never reaches EOF, + // so its read timeout fires while it is still uploading. + for (scheme, retry) in [ + ("http", false), + ("http", true), + ("https", false), + ("https", true), + ] { + let connect_timeout = scheme == "https"; + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + let (mut socket, _) = listener.accept().await.unwrap(); + let _ = tokio::io::copy(&mut socket, &mut tokio::io::sink()).await; + }); + let client = RestfulLanceDbClient::try_new( + &parse_db_url("db://test").unwrap(), + "us-east-1", + Some(format!("{scheme}://{address}")), + HeaderMap::new(), + ClientConfig { + timeout_config: TimeoutConfig { + connect_timeout: Some(Duration::from_millis(200)), + read_timeout: Some(Duration::from_secs(2)), + ..Default::default() + }, + retry_config: RetryConfig { + connect_retries: Some(1), + read_retries: Some(1), + ..Default::default() + }, + ..Default::default() + }, + None, + ) + .unwrap(); + let make_body = || { + Ok(Body::wrap_stream( + futures::stream::once(async { + Ok::<_, std::io::Error>(bytes::Bytes::from_static(b"upload")) + }) + .chain(futures::stream::pending()), + )) + }; + let request = client + .post("/v1/table/test/create/") + .header(REQUEST_ID_HEADER, "upload-timeout"); + let error = if retry { + client + .send_with_retry(request, Some(Box::new(make_body)), true) + .await + } else { + client.send(request.body(make_body().unwrap())).await + } + .unwrap_err(); + server.abort(); + + let message = error.to_string(); + assert!(message.contains("timed out"), "{message}"); + let (config, env_var) = if connect_timeout { + ( + "client_config.timeout_config.connect_timeout", + "LANCE_CLIENT_CONNECT_TIMEOUT", + ) + } else { + ( + "client_config.timeout_config.read_timeout", + "LANCE_CLIENT_READ_TIMEOUT", + ) + }; + assert!(message.contains(config), "{message}"); + assert!( + message.contains("client_config.timeout_config.timeout"), + "{message}" + ); + assert!(message.contains(env_var), "{message}"); + let (request_id, status_code, source) = match &error { + Error::Http { + request_id, + status_code, + source, + } if !retry => (request_id, status_code, source), + Error::Retry { + request_id, + status_code, + source, + read_failures, + connect_failures, + .. + } if retry => { + assert_eq!(*read_failures, u8::from(!connect_timeout)); + assert_eq!(*connect_failures, u8::from(connect_timeout)); + (request_id, status_code, source) + } + other => panic!("unexpected error: {other:?}"), + }; + assert_eq!(request_id, "upload-timeout"); + assert_eq!(*status_code, None); + // Retain the typed source for retry classification and Python causes. + let source = source.downcast_ref::().unwrap(); + assert!(source.is_timeout()); + assert_eq!(source.is_connect(), connect_timeout); + } + } + /// A configuration naming any other delimiter is refused where it was /// written, rather than producing identifiers no service splits the way the /// caller meant.