Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GraphQL] Add a separate timeout field in ServiceConfig for mutations #17742

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -3063,6 +3063,11 @@ type ServiceConfig {
"""
maxPageSize: Int!
"""
Maximum time in milliseconds that it will wait to get the results from the execution. Note,
that the transaction might be still queued for execution beyond this timeout.
stefan-mysten marked this conversation as resolved.
Show resolved Hide resolved
"""
mutationTimeoutMs: Int!
"""
Maximum time in milliseconds that will be spent to serve one request.
"""
requestTimeoutMs: Int!
Expand Down
14 changes: 14 additions & 0 deletions crates/sui-graphql-rpc/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const MAX_TYPE_NODES: u32 = 256;
const MAX_MOVE_VALUE_DEPTH: u32 = 128;

pub(crate) const DEFAULT_REQUEST_TIMEOUT_MS: u64 = 40_000;
pub(crate) const DEFAULT_MUTATION_TIMEOUT_MS: u64 = 60_000;
stefan-mysten marked this conversation as resolved.
Show resolved Hide resolved

const DEFAULT_IDE_TITLE: &str = "Sui GraphQL IDE";

Expand Down Expand Up @@ -126,6 +127,8 @@ pub struct Limits {
#[serde(default)]
pub max_page_size: u64,
#[serde(default)]
pub mutation_timeout_ms: u64,
#[serde(default)]
pub request_timeout_ms: u64,
#[serde(default)]
pub max_type_argument_depth: u32,
Expand Down Expand Up @@ -300,6 +303,12 @@ impl ServiceConfig {
self.limits.max_page_size
}

/// Maximum time in milliseconds that it will wait to get the results from the execution. Note
/// that the transaction might take longer to execute than this timeout.
stefan-mysten marked this conversation as resolved.
Show resolved Hide resolved
async fn mutation_timeout_ms(&self) -> u64 {
self.limits.mutation_timeout_ms
}

/// Maximum time in milliseconds that will be spent to serve one request.
stefan-mysten marked this conversation as resolved.
Show resolved Hide resolved
async fn request_timeout_ms(&self) -> u64 {
self.limits.request_timeout_ms
Expand Down Expand Up @@ -483,6 +492,7 @@ impl Default for Limits {
max_db_query_cost: MAX_DB_QUERY_COST,
default_page_size: DEFAULT_PAGE_SIZE,
max_page_size: MAX_PAGE_SIZE,
mutation_timeout_ms: DEFAULT_MUTATION_TIMEOUT_MS,
request_timeout_ms: DEFAULT_REQUEST_TIMEOUT_MS,
max_type_argument_depth: MAX_TYPE_ARGUMENT_DEPTH,
max_type_argument_width: MAX_TYPE_ARGUMENT_WIDTH,
Expand Down Expand Up @@ -537,6 +547,7 @@ mod tests {
max-db-query-cost = 50
default-page-size = 20
max-page-size = 50
mutation-timeout-ms = 60000
request-timeout-ms = 27000
max-type-argument-depth = 32
max-type-argument-width = 64
Expand All @@ -555,6 +566,7 @@ mod tests {
max_db_query_cost: 50,
default_page_size: 20,
max_page_size: 50,
mutation_timeout_ms: 60_000,
request_timeout_ms: 27_000,
max_type_argument_depth: 32,
max_type_argument_width: 64,
Expand Down Expand Up @@ -617,6 +629,7 @@ mod tests {
max-db-query-cost = 20
default-page-size = 10
max-page-size = 20
mutation-timeout-ms = 60000
request-timeout-ms = 30000
max-type-argument-depth = 32
max-type-argument-width = 64
Expand All @@ -638,6 +651,7 @@ mod tests {
max_db_query_cost: 20,
default_page_size: 10,
max_page_size: 20,
mutation_timeout_ms: 60_000,
request_timeout_ms: 30_000,
max_type_argument_depth: 32,
max_type_argument_width: 64,
Expand Down
38 changes: 29 additions & 9 deletions crates/sui-graphql-rpc/src/extensions/timeout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use async_graphql::{
extensions::{Extension, ExtensionContext, ExtensionFactory, NextExecute, NextParseQuery},
parser::types::ExecutableDocument,
parser::types::{ExecutableDocument, OperationType},
Response, ServerError, ServerResult,
};
use async_graphql_value::Variables;
Expand All @@ -22,12 +22,14 @@ pub(crate) struct Timeout;
#[derive(Debug, Default)]
struct TimeoutExt {
pub query: Mutex<Option<String>>,
pub is_mutation: Mutex<bool>,
}

impl ExtensionFactory for Timeout {
fn create(&self) -> Arc<dyn Extension> {
Arc::new(TimeoutExt {
query: Mutex::new(None),
is_mutation: Mutex::new(false),
stefan-mysten marked this conversation as resolved.
Show resolved Hide resolved
})
}
}
Expand All @@ -42,6 +44,13 @@ impl Extension for TimeoutExt {
next: NextParseQuery<'_>,
) -> ServerResult<ExecutableDocument> {
let document = next.run(ctx, query, variables).await?;
let is_mutation = document
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another noob q: parse_query and validation work on the entire request document right? Wouldn't the logic below set any query to is_mutation = True as long as one of the top-level operations is a mutation?

Copy link
Contributor Author

@stefan-mysten stefan-mysten May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question which made me think again about this. At least in the IDE, you cannot run both a query request and a mutation at the same time, so for mutations you have to specify mutation {executeTransactionBlock(args) {fields}}, so this would return True, and for a query { chainIdentifier } or just { chainIdentifier } would return false because the OperationType is Query.

.operations
.iter()
.any(|(_, operation)| operation.node.ty == OperationType::Mutation);
if is_mutation {
*self.is_mutation.lock().unwrap() = true;
}
stefan-mysten marked this conversation as resolved.
Show resolved Hide resolved
*self.query.lock().unwrap() = Some(ctx.stringify_execute_doc(&document, variables));
Ok(document)
}
Expand All @@ -52,10 +61,16 @@ impl Extension for TimeoutExt {
operation_name: Option<&str>,
next: NextExecute<'_>,
) -> Response {
let cfg = ctx
.data::<ServiceConfig>()
let cfg: &ServiceConfig = ctx
.data()
.expect("No service config provided in schema data");
let request_timeout = Duration::from_millis(cfg.limits.request_timeout_ms);
// increase the timeout if the request is a mutation
let request_timeout = if *self.is_mutation.lock().unwrap() {
Duration::from_millis(cfg.limits.mutation_timeout_ms)
} else {
Duration::from_millis(cfg.limits.request_timeout_ms)
};

timeout(request_timeout, next.run(ctx, operation_name))
.await
.unwrap_or_else(|_| {
Expand All @@ -74,13 +89,18 @@ impl Extension for TimeoutExt {
%error_code,
%query
);
Response::from_errors(vec![ServerError::new(
let error_msg = if *self.is_mutation.lock().unwrap() {
stefan-mysten marked this conversation as resolved.
Show resolved Hide resolved
format!(
"Mutation request timed out. Limit: {}s",
request_timeout.as_secs_f32()
)
} else {
format!(
"Request timed out. Limit: {}s",
"Query request timed out. Limit: {}s",
request_timeout.as_secs_f32()
),
None,
)])
)
};
Response::from_errors(vec![ServerError::new(error_msg, None)])
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3067,6 +3067,11 @@ type ServiceConfig {
"""
maxPageSize: Int!
"""
Maximum time in milliseconds that it will wait to get the results from the execution. Note,
that the transaction might be still queued for execution beyond this timeout.
"""
mutationTimeoutMs: Int!
"""
Maximum time in milliseconds that will be spent to serve one request.
"""
requestTimeoutMs: Int!
Expand Down Expand Up @@ -4213,3 +4218,4 @@ schema {
query: Query
mutation: Mutation
}

Loading