perf: avoid cloning global middleware list on every request#1350
perf: avoid cloning global middleware list on every request#1350
Conversation
get_global_middlewares() was returning Vec<FunctionInfo> via .to_vec(), which cloned every FunctionInfo (each requiring GIL acquisition for Py<PyAny> refcount bumps). This happened twice per request (before + after middleware). Return a RwLockReadGuard instead and iterate over borrowed references, chaining route-specific middleware via Iterator::chain. This eliminates 2*N FunctionInfo clones and GIL acquisitions per request where N is the number of global middlewares. Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe pull request optimizes middleware handling by eliminating unnecessary vector copies in the middleware router and refactoring middleware execution loops to avoid building combined mutable vectors. Instead, route-specific middleware is wrapped as 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.rs (1)
506-537:⚠️ Potential issue | 🔴 CriticalRead lock held across
.await— deadlock risk persists here.The
before_middlewaresguard (line 506-507) remains held throughout theforloop, including the.awaiton line 519. This is the downstream manifestation of the issue raised inmiddleware_router.rs. The same applies toafter_middlewaresat lines 583-617.If the fix in
middleware_router.rsis to return a clonedVec, this code will work correctly as-is since there would be no guard to hold.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.rs` around lines 506 - 537, The read lock returned by middleware_router.get_global_middlewares is being held across await points (used via before_middlewares in the for loop), causing deadlock risk; fix by ensuring you iterate over an owned collection instead of a guard—have get_global_middlewares return a cloned Vec or immediately collect/clone into a local Vec (e.g., let before_middlewares = middleware_router.get_global_middlewares(...).clone() or let before_list: Vec<_> = before_middlewares.iter().cloned().collect()), similarly materialize route_before into an owned Option<(function, route_params)> and combine it with the owned before Vec so execute_middleware_function(&request, before_middleware).await runs without holding any read lock; apply the same pattern for after_middlewares/route_after and their loop.
🤖 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/routers/middleware_router.rs`:
- Around line 93-102: The get_global_middlewares method currently returns a
std::sync::RwLockReadGuard<'_, Vec<FunctionInfo>> which causes the RwLock to be
held across await points; instead, acquire the read lock on
self.globals.get(middleware_type).unwrap(), clone the Vec<FunctionInfo> while
holding the lock, then release the lock and return the owned Vec<FunctionInfo>
(change the method signature to return Vec<FunctionInfo>); update callers in
server.rs that iterate middlewares (which previously held the guard) to use the
cloned Vec so no std::sync::RwLock is held across async .await boundaries.
---
Outside diff comments:
In `@src/server.rs`:
- Around line 506-537: The read lock returned by
middleware_router.get_global_middlewares is being held across await points (used
via before_middlewares in the for loop), causing deadlock risk; fix by ensuring
you iterate over an owned collection instead of a guard—have
get_global_middlewares return a cloned Vec or immediately collect/clone into a
local Vec (e.g., let before_middlewares =
middleware_router.get_global_middlewares(...).clone() or let before_list: Vec<_>
= before_middlewares.iter().cloned().collect()), similarly materialize
route_before into an owned Option<(function, route_params)> and combine it with
the owned before Vec so execute_middleware_function(&request,
before_middleware).await runs without holding any read lock; apply the same
pattern for after_middlewares/route_after and their loop.
🪄 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: dded83aa-4438-421b-9c90-6040372a5da2
📒 Files selected for processing (2)
src/routers/middleware_router.rssrc/server.rs
| pub fn get_global_middlewares( | ||
| &self, | ||
| middleware_type: &MiddlewareType, | ||
| ) -> std::sync::RwLockReadGuard<'_, Vec<FunctionInfo>> { | ||
| self.globals | ||
| .get(middleware_type) | ||
| .unwrap() | ||
| .read() | ||
| .unwrap() | ||
| .to_vec() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for patterns where middleware might add other middleware dynamically
rg -n "add_global_middleware|add_middleware" --type py -C 3
# Check if there are any tests that register middleware from within middleware
rg -n "middleware.*middleware" --type py -C 2Repository: sparckles/Robyn
Length of output: 7001
🏁 Script executed:
# Check server.rs for usage of get_global_middlewares
rg -n "get_global_middlewares" src/routers/server.rs -A 10 -B 2Repository: sparckles/Robyn
Length of output: 120
🏁 Script executed:
# Also check if middleware can be added after server startup
rg -n "add_global_middleware" src/ -A 3 -B 1Repository: sparckles/Robyn
Length of output: 770
🏁 Script executed:
# Verify the Python-Rust boundary and whether Python middleware can call add_global_middleware
rg -n "add_global_middleware\|add_middleware" . --type py -A 5 | head -100Repository: sparckles/Robyn
Length of output: 41
🏁 Script executed:
# Find where get_global_middlewares is used in server.rs
rg -n "get_global_middlewares" src/server.rs -A 15 -B 3Repository: sparckles/Robyn
Length of output: 1960
🏁 Script executed:
# Get context around the line numbers mentioned (519, 590-595)
sed -n '510,530p' src/server.rs
sed -n '580,610p' src/server.rsRepository: sparckles/Robyn
Length of output: 2510
🏁 Script executed:
# Check the middleware execution function to see if it can trigger registration
rg -n "execute_middleware_function|execute_after_middleware_function" src/ -A 10 | head -150Repository: sparckles/Robyn
Length of output: 3446
🏁 Script executed:
# Check if add_global_middleware is exposed to Python bindings
rg -n "add_global_middleware" . --type rs | grep -E "pub.*fn|#\[pymethods\]|#\[pyo3\]" -B 2 -A 1Repository: sparckles/Robyn
Length of output: 85
🏁 Script executed:
# Look for Python FFI/PyO3 bindings
rg -n "add_global_middleware\|pymethods" . --type rs -B 5 -A 3 | head -80Repository: sparckles/Robyn
Length of output: 85
🏁 Script executed:
# Check if middleware has access to server instance in Python
fd -e py . | xargs grep -l "server\." | head -5
cat robyn/router.py | grep -A 20 "def before_request\|def after_request" | head -40Repository: sparckles/Robyn
Length of output: 127
🏁 Script executed:
# Search for PyO3 bindings of add_global_middleware
rg "add_global_middleware" src/ -B 3 -A 3 | grep -E "pub fn|#\[" -B 2 -A 2Repository: sparckles/Robyn
Length of output: 679
🏁 Script executed:
# Check if Server is exposed to Python bindings
rg "#\[pymethods\]" src/ -A 30 | head -100Repository: sparckles/Robyn
Length of output: 5401
🏁 Script executed:
# Look at the server.rs bindings to see what's exposed to Python
rg "fn add_global_middleware\|#\[pyo3\]\|#\[pymethods\]" src/server.rs -B 2 -A 5Repository: sparckles/Robyn
Length of output: 41
🏁 Script executed:
# Check if Python middleware can call Robyn API methods at runtime
grep -r "self.app\|self.server\|add_global_middleware\|add_middleware" robyn/ --include="*.py" | grep -v "def " | head -20Repository: sparckles/Robyn
Length of output: 757
🏁 Script executed:
# Check if Server struct has pymethods
rg "#\[pymethods\]|#\[pyclass\]" src/server.rs -A 10 | head -50Repository: sparckles/Robyn
Length of output: 923
🏁 Script executed:
# Look at the Server struct definition to see if it's a PyClass
rg "pub struct Server|#\[pyclass\]" src/server.rs -B 2 -A 5Repository: sparckles/Robyn
Length of output: 289
🏁 Script executed:
# Check the actual current return type by looking at the method signature more carefully
sed -n '93,102p' src/routers/middleware_router.rsRepository: sparckles/Robyn
Length of output: 336
🏁 Script executed:
# Check if there are any other callers of get_global_middlewares that might be affected
rg "get_global_middlewares" src/ -B 5 -A 10Repository: sparckles/Robyn
Length of output: 2807
🏁 Script executed:
# Check the pymethods block for Server to see what methods are exposed to Python
rg "#\[pymethods\]" src/server.rs -A 200 | head -250Repository: sparckles/Robyn
Length of output: 10413
🏁 Script executed:
# Specifically look for add_global_middleware in the pymethods section
sed -n '30,500p' src/server.rs | grep -A 5 "add_global_middleware"Repository: sparckles/Robyn
Length of output: 383
🏁 Script executed:
# Check if there's any way for Python middleware to access the Server instance
rg "FunctionInfo|execute_middleware_function" src/executors/mod.rs -B 5 -A 15 | head -80Repository: sparckles/Robyn
Length of output: 2665
🏁 Script executed:
# Verify that add_global_middleware is NOT in the pymethods block
sed -n '25,450p' src/server.rs | grep -E "pub fn|add_global_middleware" | head -30Repository: sparckles/Robyn
Length of output: 656
Holding std::sync::RwLock across async .await is an anti-pattern that can cause thread starvation.
While the returned RwLockReadGuard is held by callers in server.rs during the entire middleware iteration loop including across .await points, std::sync::RwLock is not designed for async contexts. It blocks the underlying OS thread rather than yielding to the async runtime, which can starve the executor when the lock is held across .await boundaries.
Consider one of these alternatives:
- Clone the
Vecunder the lock to release it before async iteration (restores original behavior) - Use
tokio::sync::RwLockwhich is designed for async contexts - Collect middleware into a local
Vecbefore iterating
Proposed fix: Clone inside lock to release before async iteration
pub fn get_global_middlewares(
&self,
middleware_type: &MiddlewareType,
- ) -> std::sync::RwLockReadGuard<'_, Vec<FunctionInfo>> {
+ ) -> Vec<FunctionInfo> {
self.globals
.get(middleware_type)
.unwrap()
.read()
.unwrap()
+ .clone()
}📝 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.
| pub fn get_global_middlewares( | |
| &self, | |
| middleware_type: &MiddlewareType, | |
| ) -> std::sync::RwLockReadGuard<'_, Vec<FunctionInfo>> { | |
| self.globals | |
| .get(middleware_type) | |
| .unwrap() | |
| .read() | |
| .unwrap() | |
| .to_vec() | |
| } | |
| pub fn get_global_middlewares( | |
| &self, | |
| middleware_type: &MiddlewareType, | |
| ) -> Vec<FunctionInfo> { | |
| self.globals | |
| .get(middleware_type) | |
| .unwrap() | |
| .read() | |
| .unwrap() | |
| .clone() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routers/middleware_router.rs` around lines 93 - 102, The
get_global_middlewares method currently returns a std::sync::RwLockReadGuard<'_,
Vec<FunctionInfo>> which causes the RwLock to be held across await points;
instead, acquire the read lock on self.globals.get(middleware_type).unwrap(),
clone the Vec<FunctionInfo> while holding the lock, then release the lock and
return the owned Vec<FunctionInfo> (change the method signature to return
Vec<FunctionInfo>); update callers in server.rs that iterate middlewares (which
previously held the guard) to use the cloned Vec so no std::sync::RwLock is held
across async .await boundaries.
1 participant
Summary
get_global_middlewares()returnedVec<FunctionInfo>via.to_vec(), cloning everyFunctionInfo(each requiring GIL acquisition forPy<PyAny>refcount bumps). Called twice per request (before + after middleware).RwLockReadGuard<Vec<FunctionInfo>>and the server iterates over borrowed references, chaining route-specific middleware viaIterator::chain.FunctionInfoclones and GIL acquisitions per request where N is the number of global middlewares.Test plan
check_responsehelper validatesglobal_afterheader is present on every response, confirming after-middleware still runsMade with Cursor
Summary by CodeRabbit
Release Notes