Skip to content

perf: avoid cloning global middleware list on every request #1350

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.

Sign up for GitHub

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

Open
sansyrox wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf: avoid cloning global middleware list on every request #1350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/routers/middleware_router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,15 @@ impl MiddlewareRouter {
Ok(())
}

pub fn get_global_middlewares(&self, middleware_type: &MiddlewareType) -> Vec<FunctionInfo> {
pub fn get_global_middlewares(
&self,
middleware_type: &MiddlewareType,
) -> std::sync::RwLockReadGuard<'_, Vec<FunctionInfo>> {
self.globals
.get(middleware_type)
.unwrap()
.read()
.unwrap()
.to_vec()
}
Comment on lines +93 to 102
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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 2

Repository: 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 2

Repository: 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 1

Repository: 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 -100

Repository: 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 3

Repository: 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.rs

Repository: 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 -150

Repository: 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 1

Repository: 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 -80

Repository: 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 -40

Repository: 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 2

Repository: sparckles/Robyn

Length of output: 679

🏁 Script executed:

# Check if Server is exposed to Python bindings
rg "#\[pymethods\]" src/ -A 30 | head -100

Repository: 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 5

Repository: 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 -20

Repository: sparckles/Robyn

Length of output: 757

🏁 Script executed:

# Check if Server struct has pymethods
rg "#\[pymethods\]|#\[pyclass\]" src/server.rs -A 10 | head -50

Repository: 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 5

Repository: 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.rs

Repository: 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 10

Repository: 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 -250

Repository: 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 -80

Repository: 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 -30

Repository: 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:

  1. Clone the Vec under the lock to release it before async iteration (restores original behavior)
  2. Use tokio::sync::RwLock which is designed for async contexts
  3. Collect middleware into a local Vec before 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.

Suggested change
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.

Copy link
Copy Markdown
Member Author

@sansyrox sansyrox Apr 12, 2026

Choose a reason for hiding this comment

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

Resolve — reviewed, the middleware optimization is correct. Dynamic middleware addition is not a supported pattern.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 12, 2026

Choose a reason for hiding this comment

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

Rate Limit Exceeded

@sansyrox have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 53 seconds before sending another message.


pub fn has_any_middleware(&self) -> bool {
Expand Down
22 changes: 10 additions & 12 deletions src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,13 +509,14 @@ async fn index(

let mut early_response: Option<Response> = None;
if !before_middlewares.is_empty() || route_before.is_some() {
let mut all_before = before_middlewares;
if let Some((function, route_params)) = route_before {
all_before.push(function);
let route_fn = if let Some((function, route_params)) = route_before {
request.path_params = route_params;
}
for before_middleware in all_before {
request = match execute_middleware_function(&request, &before_middleware).await {
Some(function)
} else {
None
};
for before_middleware in before_middlewares.iter().chain(route_fn.as_ref()) {
request = match execute_middleware_function(&request, before_middleware).await {
Ok(MiddlewareReturn::Request(r)) => r,
Ok(MiddlewareReturn::Response(r)) => {
early_response = Some(r);
Expand Down Expand Up @@ -583,16 +584,13 @@ async fn index(
let route_after = middleware_router.get_route(&MiddlewareType::AfterRequest, &route);

if !after_middlewares.is_empty() || route_after.is_some() {
let mut all_after = after_middlewares;
if let Some((function, _)) = route_after {
all_after.push(function);
}
for after_middleware in all_after {
let route_fn = route_after.map(|(function, _)| function);
for after_middleware in after_middlewares.iter().chain(route_fn.as_ref()) {
if let ResponseType::Standard(std_response) = response {
response = match execute_after_middleware_function(
&request,
&std_response,
&after_middleware,
after_middleware,
)
.await
{
Expand Down
Loading