mirror of
https://github.com/lancedb/lancedb.git
synced 2026-10-28 22:46:14 +00:00
fix(remote): make HTTP timeout errors actionable (#4650)
Remote `create_table` reader uploads can exceed the read deadline while waiting for response headers, but reqwest's main error message only says "error sending request". Make HTTP and retry errors explicitly identify timeouts and name the relevant `client_config.timeout_config` settings and environment variables, preserving typed sources, request metadata, and retry classification. Document that the read timeout covers waiting for headers during upload. This implements the issue's diagnostic option: creation still uses one streaming request, so large or slow readers may require a longer `client_config.timeout_config.read_timeout`. Validation: - 48 focused Rust client, error, and retry tests passed. Regression coverage holds an upload open until its read timeout and also exercises TLS connection timeouts, with and without retries. - 8 Python create-table and HTTP/retry error tests passed in the uv-managed environment after rebuilding the extension, including reader creation with read and overall timeouts. - `cargo check --quiet --features remote --tests --examples` passed. - `cargo clippy --quiet --features remote --tests --examples` passed. - Rust formatting and Python package Ruff lint/format checks passed. - Repository-wide `ruff check .` reports 49 existing findings under `docs/web-tests`; those files are unchanged. Fixes #4646 <!-- lance-gatekeeper-fix:v1 agent=ce1c040fa581f1748166ecb686d9c9c0 generation=1 --> --------- Co-authored-by: Gatefixer <313497061+lancedb-gatefixer[bot]@users.noreply.github.com> Co-authored-by: Xuanwo <github@xuanwo.io>
This commit is contained in:
co-authored by
Xuanwo
parent
ad1212fb32
commit
83b7275137
@@ -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?
|
||||
|
||||
@@ -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<f64>,
|
||||
/// 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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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}
|
||||
|
||||
@@ -10,6 +10,25 @@ use snafu::Snafu;
|
||||
|
||||
pub(crate) type BoxError = Box<dyn std::error::Error + Send + Sync>;
|
||||
|
||||
// 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::<reqwest::Error>() {
|
||||
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<dyn std::error::Error + Send + Sync>,
|
||||
@@ -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<candle_core::Error> 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 {
|
||||
|
||||
@@ -233,6 +233,10 @@ pub struct TimeoutConfig {
|
||||
pub connect_timeout: Option<Duration>,
|
||||
/// 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::<reqwest::Error>().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.
|
||||
|
||||
Reference in New Issue
Block a user