chore: clean code - #90
Conversation
📝 WalkthroughWalkthroughThe PR refactors request processing around shared ChangesRequest dispatch refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConnectionHandler
participant process_requests
participant MiddlewareChain
participant PythonHandler
participant ResponseSender
ConnectionHandler->>process_requests: submit ProcessRequest
process_requests->>MiddlewareChain: execute route middleware
MiddlewareChain->>PythonHandler: invoke wrapped route handler
PythonHandler-->>process_requests: return handler result or error
process_requests->>ResponseSender: send converted response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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: 3
🧹 Nitpick comments (3)
tests/app.py (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a return type hint to
main.-def main(): +def main() -> None:As per path instructions: "Use type hints for function signatures in Python tests and handlers".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/app.py` at line 9, Add the appropriate return type hint to the `main` function signature in `tests/app.py`, following the project’s existing Python test and handler typing conventions.Source: Path instructions
src/request.rs (1)
269-269: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSize the response channel for a single message.
Each request allocates an mpsc channel with
ctx.channel_capacityslots. Exactly oneResponseis ever sent on it. Usetokio::sync::oneshotinstead, or pass capacity1. This removes a per-request allocation proportional tochannel_capacity.Also applies to: 288-288
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/request.rs` at line 269, Update the response channel creation in the request handling paths around response_sender and the corresponding second occurrence to use a single-message channel: prefer tokio::sync::oneshot when the sender/receiver API is compatible, otherwise set the mpsc capacity to 1 instead of ctx.channel_capacity. Preserve the existing one-response send and receive behavior.src/middleware.rs (1)
20-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd doc comments to the public
MiddlewareChain::executeAPI.
MiddlewareChainandexecuteare public and now expose a new signature. Add///documentation with Args, Returns, and Example sections.As per coding guidelines: "Use doc comments
///for public APIs in Rust" and "Include Args, Returns, and Example sections in Rust docstrings for public APIs".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/middleware.rs` around lines 20 - 37, Add Rust /// documentation to the public MiddlewareChain type and its execute method, including Args, Returns, and Example sections that describe the middleware execution API and its PyResult return value. Keep the implementation unchanged and document the exposed parameters, route handler, arguments, and keyword arguments.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib.rs`:
- Around line 505-512: Handle the `RequestBuilder::build()` error in the request
processing chain instead of calling `.unwrap()`, returning a 500 response when
body collection or multipart parsing fails. Preserve the existing
`.process(ctx).await` flow for successfully built requests.
- Around line 521-539: Update process_requests so each received ProcessRequest
is handled in a spawned task rather than awaiting call_python_handler inline,
allowing the receiver loop to continue accepting requests concurrently. Move
response construction and response_sender.send into the per-request task, while
preserving shutdown handling and existing error-to-Response conversion.
In `@src/request.rs`:
- Around line 263-282: Update handle_found_route to stop transmuting
MatchRoute<'_> into MatchRoute<'static> before constructing ProcessRequest.
Convert the route parameters into owned strings and retain only owned/shared
route metadata required by call_python_handler, such as the handler and route
sequence. Adjust ProcessRequest and downstream consumers to use these owned
values, eliminating borrowed route data from the queued request.
---
Nitpick comments:
In `@src/middleware.rs`:
- Around line 20-37: Add Rust /// documentation to the public MiddlewareChain
type and its execute method, including Args, Returns, and Example sections that
describe the middleware execution API and its PyResult return value. Keep the
implementation unchanged and document the exposed parameters, route handler,
arguments, and keyword arguments.
In `@src/request.rs`:
- Line 269: Update the response channel creation in the request handling paths
around response_sender and the corresponding second occurrence to use a
single-message channel: prefer tokio::sync::oneshot when the sender/receiver API
is compatible, otherwise set the mpsc capacity to 1 instead of
ctx.channel_capacity. Preserve the existing one-response send and receive
behavior.
In `@tests/app.py`:
- Line 9: Add the appropriate return type hint to the `main` function signature
in `tests/app.py`, following the project’s existing Python test and handler
typing conventions.
🪄 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 Plus
Run ID: 799de851-da10-436b-ac6e-6c62a4dd1c0e
📒 Files selected for processing (5)
src/lib.rssrc/middleware.rssrc/request.rssrc/routing.rstests/app.py
| RequestBuilder::new(req) | ||
| .with_app_data(&ctx.app_data) | ||
| .with_template(&ctx.template) | ||
| .build() | ||
| .await | ||
| .unwrap(); | ||
| request.process(ctx).await | ||
| .unwrap() | ||
| .process(ctx) | ||
| .await |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Remove the .unwrap() on RequestBuilder::build(); a remote client can panic the connection task.
build() returns PyResult<Request> and fails on conditions that a client controls. src/request.rs lines 350-381 show two failure paths: self.req.collect().await.into_py_exception()? fails when the body read fails, for example on an aborted upload, and parse_multipart(...).into_py_exception()? fails on a malformed multipart/form-data body. Each failure now panics inside the hyper service task instead of producing a response.
Return a 500 response for the error case.
🛡️ Proposed fix
hyper::service::service_fn(move |req| {
let ctx = ctx.clone();
async move {
- RequestBuilder::new(req)
+ let request = RequestBuilder::new(req)
.with_app_data(&ctx.app_data)
.with_template(&ctx.template)
.build()
- .await
- .unwrap()
- .process(ctx)
- .await
+ .await;
+ match request {
+ Ok(request) => request.process(ctx).await,
+ Err(_) => Response::from(Status::INTERNAL_SERVER_ERROR).try_into(),
+ }
}
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RequestBuilder::new(req) | |
| .with_app_data(&ctx.app_data) | |
| .with_template(&ctx.template) | |
| .build() | |
| .await | |
| .unwrap(); | |
| request.process(ctx).await | |
| .unwrap() | |
| .process(ctx) | |
| .await | |
| let request = RequestBuilder::new(req) | |
| .with_app_data(&ctx.app_data) | |
| .with_template(&ctx.template) | |
| .build() | |
| .await; | |
| match request { | |
| Ok(request) => request.process(ctx).await, | |
| Err(_) => Response::from(Status::INTERNAL_SERVER_ERROR).try_into(), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib.rs` around lines 505 - 512, Handle the `RequestBuilder::build()`
error in the request processing chain instead of calling `.unwrap()`, returning
a 500 response when body collection or multipart parsing fails. Preserve the
existing `.process(ctx).await` flow for successfully built requests.
| async fn process_requests( | ||
| &self, | ||
| mut shutdown: ShutDownSignal, | ||
| mut rx: Receiver<ProcessRequest>, | ||
| mut request_receiver: Receiver<ProcessRequest>, | ||
| ) -> PyResult<()> { | ||
| loop { | ||
| tokio::select! { | ||
| Some(req) = rx.recv() => self.handle_request(req).await?, | ||
| Some(pr) = request_receiver.recv() => { | ||
| let response = call_python_handler(&pr.middlewares, &pr.match_route, &pr.request, self.is_async) | ||
| .await | ||
| .unwrap_or_else(Response::from) | ||
| .call_wrapper(&pr); | ||
| let _ = pr.response_sender.send(response).await; | ||
| }, | ||
| _ = shutdown.wait() => break, | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the previous dispatch loop and any task spawning around request processing.
rg -n -C6 'process_requests|tokio::spawn|call_python_handler' src/lib.rs
git log --oneline -5 -- src/lib.rsRepository: j03-dev/oxapy
Length of output: 2983
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== struct and relevant fields =="
rg -n -C4 'struct .*|is_async|max_connections|channel_capacity|request_sender|ProcessRequest|send\(request' src/lib.rs
echo
echo "== process_requests surrounding code =="
sed -n '430,545p' src/lib.rs | nl -ba -v430
echo
echo "== call_python_handler implementation =="
sed -n '542,620p' src/lib.rs | nl -ba -v542Repository: j03-dev/oxapy
Length of output: 6546
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== call_python_handler implementation =="
sed -n '542,585p' src/lib.rs
echo
echo "== request sending locations =="
rg -n -C5 'request_sender|ProcessRequest|response_receiver|recv\\(' src/lib.rs
echo
echo "== deterministic model: one process_requests task awaiting Python handler keeps loop at one in-flight request =="
python3 - <<'PY'
events = []
pr_channel = []
permitted_connections = 3
process_requests_in_flight = 0
def event(s): events.append(s)
def spawn_connection_accept():
pr_channel.append('req')
event("accept_and_send_request")
def process_request_batch():
global process_requests_in_flight
if pr_channel:
pr_channel.pop(0)
event("process_requests_recieve_and_await_handler_started")
process_requests_in_flight += 1
# one await point; while handler awaits, no next recv occurs.
event("handler_awaits_returns")
process_requests_in_flight -= 1
event("process_requests_responds_recieve_next_request")
spawn_connection_accept()
spawn_connection_accept()
spawn_connection_accept()
process_request_batch()
process_request_batch()
process_request_batch()
print("requests_sent:", len([e for e in events if e == "accept_and_send_request"]))
print("requests_in_channel:", len(pr_channel))
print("process_requests_in_flight:", process_requests_in_flight)
print("events:", events)
PYRepository: j03-dev/oxapy
Length of output: 1770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== request sending and response waiting locations =="
rg -n -C5 'request_sender|ProcessRequest|response_receiver|recv\\(' src/lib.rs || true
echo
echo "== pyo3 detach helpers in file =="
rg -n -C4 'py.detach|block_on|future_into_py|Python::attach|into_future|allow_threads' src/lib.rs
echo
echo "== imports / no_std context hints =="
sed -n '1,80p' src/lib.rsRepository: j03-dev/oxapy
Length of output: 4602
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all ProcessRequest/request_sender usages =="
grep -R -n "ProcessRequest\|request_sender" src || true
echo
echo "== HttpServer request handlers and channel buffering =="
# print matching context around likely request entry points and Channel/Sender fields, line by line
python3 - <<'PY'
from pathlib import Path
p = Path('src/lib.rs')
text = p.read_text().splitlines()
targets = ['request_sender', 'ProcessRequest', 'ctx.request_sender.send', 'response_receiver', 'recv(']
seen = set()
for i,l in enumerate(text, start=1):
if any(t in l for t in targets):
start=max(1,i-20); end=min(len(text),i+35)
key=(start,end)
if key in seen: continue
seen.add(key)
print(f"--- src/lib.rs:{i} ---")
for n in range(start,end+1):
print(f"{n:4}: {text[n-1]}")
PYRepository: j03-dev/oxapy
Length of output: 16060
Spawn each request instead of awaiting the Python handler inline.
process_requests receives from the channel, then awaits call_python_handler(...) before receiving the next request. This makes channel_capacity only a queue limit and caps in-flight request concurrency to 1 for async handlers, while I/O-bound handlers block the only consumer. Spawn per-ProcessRequest, then await its response only after sending it or with per-request capacity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib.rs` around lines 521 - 539, Update process_requests so each received
ProcessRequest is handled in a spawned task rather than awaiting
call_python_handler inline, allowing the receiver loop to continue accepting
requests concurrently. Move response construction and response_sender.send into
the per-request task, while preserving shutdown handling and existing
error-to-Response conversion.
| async fn handle_found_route( | ||
| &self, | ||
| ctx: &RequestContext, | ||
| router: Arc<Router>, | ||
| ctx: &Context, | ||
| match_route: MatchRoute<'_>, | ||
| middlewares: Option<Arc<[Middleware]>>, | ||
| ) -> Result<hyper::Response<Body>, hyper::http::Error> { | ||
| let (tx, rx) = tokio::sync::mpsc::channel(ctx.channel_capacity); | ||
| let (response_sender, response_receiver) = tokio::sync::mpsc::channel(ctx.channel_capacity); | ||
|
|
||
| let transmutate_route: MatchRoute<'static> = unsafe { std::mem::transmute(match_route) }; | ||
|
|
||
| let process_request = ProcessRequest { | ||
| request: Arc::new(self.clone()), | ||
| router: Some(router), | ||
| match_route: Some(transmutate_route), | ||
| tx, | ||
| middlewares, | ||
| request: Arc::new(self.clone()), | ||
| response_sender, | ||
| wrapper: ctx.wrapper.clone(), | ||
| }; | ||
|
|
||
| Self::send_and_wait_response(ctx, process_request, rx).await | ||
| Self::send_and_wait_response(&ctx.request_sender, process_request, response_receiver).await | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm which fields of match_route the dispatch loop reads, and how params are consumed.
rg -n -C6 'match_route|build_route_params' src/lib.rs
rg -n -C4 'pub type MatchRoute|struct Route\b' src/routing.rsRepository: j03-dev/oxapy
Length of output: 2846
🏁 Script executed:
#!/bin/bash
set -e
echo "== candidate files around Request and routing fields =="
wc -l src/request.rs src/routing.rs src/lib.rs
echo
echo "== src/request.rs relevant sections =="
sed -n '220,315p' src/request.rs | cat -n
echo
echo "== src/routing.rs Route definition and matches =="
sed -n '1,80p' src/routing.rs | cat -n
echo
echo "== src/lib.rs ProcessRequest/send/response handling =="
sed -n '35,70p' src/lib.rs | cat -n
sed -n '460,545p' src/lib.rs | cat -nRepository: j03-dev/oxapy
Length of output: 11553
🏁 Script executed:
#!/bin/bash
set -e
echo "== RequestBuilder URI handling =="
rg -n -C4 'struct RequestBuilder|impl RequestBuilder|uri|method|find\(' src/request.rs src/routing.rs src/lib.rs
echo "== matchit::Match fields/params usage =="
rg -n 'matchit::Match|match_route\.params|match_route\.value|params\.iter' src/*.rsRepository: j03-dev/oxapy
Length of output: 14534
🌐 Web query:
matchit::Match params lifetime value borrowed from input documentation
💡 Result:
In the matchit crate, the Match<'k, 'v, V> struct and its associated Params<'k, 'v> structure utilize lifetime parameters to represent that the URL parameters (keys and values) are borrowed from the input path string used during the routing process [1][2][3]. Specifically: - The 'k lifetime represents the lifetime of the parameter keys [3]. - The 'v lifetime represents the lifetime of the parameter values [3]. - When a Router::at call is performed, the resulting Match object contains a Params instance [2]. These parameters are slices (&'v str when accessed via get) that point directly into the input path string [1][3]. Because these parameters are references to the input path, the Match object cannot outlive the input string from which the path was parsed [4]. This design allows matchit to be a zero-copy router, avoiding allocations during the matching process by simply referencing portions of the input [3][5]. If you need to store the parameters independently of the input path, you must convert them to owned types (e.g., String) or use an alternative storage mechanism, as the borrowed references are strictly bound to the lifetime of the input [4].
Citations:
- 1: https://docs.rs/matchit/latest/matchit/struct.Params.html
- 2: https://docs.rs/matchit/latest/matchit/struct.Match.html
- 3: https://doc.cuprate.org/src/matchit/params.rs.html
- 4: use reference counted prefix for less lifetimes on params? ibraheemdev/matchit#28
- 5: https://docs.rs/matchit/latest/matchit/
Avoid storing borrowed route matches in ProcessRequest.
MatchRoute borrows self.uri.path() as route params and ctx.routers entries from Router::find; handle_found_route erases both lifetimes at src/request.rs:271 and sends the value into the global request queue. If Hyper cancels process because the connection drops, the service future is dropped while rx.recv().await is still running. A queued ProcessRequest can then survive and call_python_handler later reads params.iter(), which is invalid if the borrowed input lifetimes have ended. Remove the transmute and store only owned data needed by Python: path parameters as owned strings, plus shared route metadata such as the handler and route sequence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/request.rs` around lines 263 - 282, Update handle_found_route to stop
transmuting MatchRoute<'_> into MatchRoute<'static> before constructing
ProcessRequest. Convert the route parameters into owned strings and retain only
owned/shared route metadata required by call_python_handler, such as the handler
and route sequence. Adjust ProcessRequest and downstream consumers to use these
owned values, eliminating borrowed route data from the queued request.
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor