fix: wire client_timeout and keep_alive_timeout through to Actix#1348
fix: wire client_timeout and keep_alive_timeout through to Actix#1348
Conversation
The Python API accepts client_timeout and keep_alive_timeout parameters and threads them all the way to spawn_process, but they were never passed to Server.start(). The Rust side hardcoded KeepAlive::Os and client_request_timeout of 0 seconds, silently ignoring user config. Pass both values through to Server.start() and use them when building the Actix HttpServer. Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR exposes server timeout parameters end-to-end: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server.rs`:
- Around line 280-282: The current use of client_request_timeout only affects
request header read time and does not stop long-running handlers; update the
server to enforce handler execution timeouts by adding a middleware or wrapping
handler execution with a timeout (e.g., add a tower::timeout::TimeoutLayer or
apply tokio::time::timeout around request handling) instead of relying on
client_request_timeout; locate where the server is configured (the block using
KeepAlive::Timeout, .workers, .client_request_timeout) and wrap the service or
router with a timeout layer (or modify the request-handling entrypoint to use
tokio::time::timeout) so that handler functions are aborted when they exceed the
desired duration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b8934a6-de98-489b-9fc9-93150212c9df
📒 Files selected for processing (2)
robyn/processpool.pysrc/server.rs
| .keep_alive(KeepAlive::Timeout(std::time::Duration::from_secs(keep_alive_timeout))) | ||
| .workers(workers) | ||
| .client_request_timeout(std::time::Duration::from_secs(0)) | ||
| .client_request_timeout(std::time::Duration::from_secs(client_timeout)) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In actix-web HttpServer, what exactly does client_request_timeout(Duration) control? Does it terminate a request when the handler itself runs longer than the timeout (e.g., handler sleeps 10 seconds)? Please cite official actix-web docs.
💡 Result:
In actix-web's HttpServer, client_request_timeout(Duration) controls the timeout for reading the client request head (headers). If the client does not transmit the entire set of headers within this time, the request is terminated with a 408 (Request Timeout) error. The default is 5000 milliseconds (5 seconds). To disable, set to Duration::ZERO. It does NOT terminate a request when the handler itself runs longer than the timeout (e.g., a handler that sleeps 10 seconds). This timeout only applies to initial header reading, not handler execution time.
Citations:
client_request_timeout does not enforce handler execution timeout (Line 282).
The manual test assumes that client_request_timeout will terminate requests when the handler runs longer than the timeout (e.g., handler sleeps 10 seconds). However, client_request_timeout only controls the timeout for reading client request headers, not handler execution time. A handler that sleeps 10 seconds will not be terminated by this setting. To enforce handler execution timeouts, a different mechanism is required (e.g., middleware-based timeout or tokio::time::timeout).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server.rs` around lines 280 - 282, The current use of
client_request_timeout only affects request header read time and does not stop
long-running handlers; update the server to enforce handler execution timeouts
by adding a middleware or wrapping handler execution with a timeout (e.g., add a
tower::timeout::TimeoutLayer or apply tokio::time::timeout around request
handling) instead of relying on client_request_timeout; locate where the server
is configured (the block using KeepAlive::Timeout, .workers,
.client_request_timeout) and wrap the service or router with a timeout layer (or
modify the request-handling entrypoint to use tokio::time::timeout) so that
handler functions are aborted when they exceed the desired duration.
There was a problem hiding this comment.
Resolve — informational comment about Actix semantics, no code change needed.
There was a problem hiding this comment.
Rate Limit Exceeded
@sansyrox have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 49 seconds before sending another message.
…dler execution The `client_timeout` parameter maps to actix-web's `client_request_timeout`, which only controls how long the server waits for the client to transmit the complete request headers. It does NOT limit handler execution time or overall request duration. Updated docstrings, type stubs, Rust doc comments, and both English and Chinese documentation to make this clear and prevent user confusion. Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
robyn/processpool.py (1)
16-33: Add a fail-fast guard for invalid timeout values.Since these values now materially affect startup, validate them upfront (
>= 0) to avoid opaque failures later in worker startup paths.Suggested patch
def run_processes( @@ ) -> List[Process]: + if client_timeout < 0 or keep_alive_timeout < 0: + raise ValueError( + "client_timeout and keep_alive_timeout must be non-negative integers" + ) + """ Spawn server processes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@robyn/processpool.py` around lines 16 - 33, Add a fail-fast check at the start of run_processes to validate the timeout parameters: verify client_timeout and keep_alive_timeout are integers >= 0 and raise a clear ValueError (or TypeError for non-int types) that includes the parameter name and the invalid value; this prevents later opaque failures during worker startup and makes debugging immediate when run_processes is called with bad timeout values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@robyn/processpool.py`:
- Around line 16-33: Add a fail-fast check at the start of run_processes to
validate the timeout parameters: verify client_timeout and keep_alive_timeout
are integers >= 0 and raise a clear ValueError (or TypeError for non-int types)
that includes the parameter name and the invalid value; this prevents later
opaque failures during worker startup and makes debugging immediate when
run_processes is called with bad timeout values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbe8e251-f485-434f-b346-b85f233b005e
📒 Files selected for processing (6)
docs_src/src/pages/documentation/en/api_reference/timeout_configuration.mdxdocs_src/src/pages/documentation/zh/api_reference/timeout_configuration.mdxrobyn/__init__.pyrobyn/processpool.pyrobyn/robyn.pyisrc/server.rs
✅ Files skipped from review due to trivial changes (3)
- robyn/robyn.pyi
- docs_src/src/pages/documentation/en/api_reference/timeout_configuration.mdx
- robyn/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server.rs
1 participant
Summary
client_timeoutandkeep_alive_timeoutparameters and threads them throughrun_processes→spawn_process, butspawn_processonly calledserver.start(socket, workers)— silently dropping both timeout values.Server::starthardcodedKeepAlive::Osandclient_request_timeout(Duration::from_secs(0)), meaning the documented timeout configuration had zero effect.Server::startsignature and passes them to Actix'sHttpServerbuilder.Test plan
client_timeout=2, send a request to a handler that sleeps 10s, verify the server cuts it offkeep_alive_timeoutcauses idle connections to be closed after the configured durationMade with Cursor
Summary by CodeRabbit
New Features
Documentation