-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Avoid some extra clones/allocations #2865
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -331,9 +331,11 @@ where | |
} | ||
} | ||
|
||
let path = req.uri().path().to_owned(); | ||
// split apart to get multiple mutable references to parts, | ||
// this avoids the need to clone the URI/path. | ||
let (mut parts, body) = req.into_parts(); | ||
|
||
match self.node.at(&path) { | ||
match self.node.at(parts.uri.path()) { | ||
Ok(match_) => { | ||
let id = *match_.value; | ||
|
||
|
@@ -342,22 +344,24 @@ where | |
crate::extract::matched_path::set_matched_path_for_request( | ||
id, | ||
&self.node.route_id_to_path, | ||
req.extensions_mut(), | ||
&mut parts.extensions, | ||
); | ||
} | ||
|
||
url_params::insert_url_params(req.extensions_mut(), match_.params); | ||
url_params::insert_url_params(&mut parts.extensions, match_.params); | ||
|
||
let endpoint = self | ||
.routes | ||
.get(&id) | ||
.expect("no route for id. This is a bug in axum. Please file an issue"); | ||
|
||
let req = Request::from_parts(parts, body); | ||
|
||
match endpoint { | ||
Endpoint::MethodRouter(method_router) => { | ||
Ok(method_router.call_with_state(req, state)) | ||
} | ||
Endpoint::Route(route) => Ok(route.clone().call(req)), | ||
Endpoint::Route(route) => Ok(route.clone().call_owned(req)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is not reflected in any tests. Do you think we could test this? |
||
} | ||
} | ||
// explicitly handle all variants in case matchit adds | ||
|
@@ -366,7 +370,7 @@ where | |
MatchError::NotFound | ||
| MatchError::ExtraTrailingSlash | ||
| MatchError::MissingTrailingSlash, | ||
) => Err((req, state)), | ||
) => Err((Request::from_parts(parts, body), state)), | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,13 +42,27 @@ impl<E> Route<E> { | |
))) | ||
} | ||
|
||
/// Variant of [`Route::call`] that takes ownership of the route to avoid cloning. | ||
pub(crate) fn call_owned(self, req: Request<Body>) -> RouteFuture<E> { | ||
let req = req.map(Body::new); | ||
RouteFuture::from_future(self.oneshot_inner_owned(req)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When comparing with I'm not sure about the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a note for me. |
||
} | ||
|
||
pub(crate) fn oneshot_inner( | ||
&mut self, | ||
req: Request, | ||
) -> Oneshot<BoxCloneService<Request, Response, E>, Request> { | ||
self.0.get_mut().unwrap().clone().oneshot(req) | ||
} | ||
|
||
/// Variant of [`Route::oneshot_inner`] that takes ownership of the route to avoid cloning. | ||
pub(crate) fn oneshot_inner_owned( | ||
self, | ||
req: Request, | ||
) -> Oneshot<BoxCloneService<Request, Response, E>, Request> { | ||
self.0.into_inner().unwrap().oneshot(req) | ||
} | ||
|
||
pub(crate) fn layer<L, NewError>(self, layer: L) -> Route<NewError> | ||
where | ||
L: Layer<Route<E>> + Clone + Send + 'static, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no tests covering this change. Do you think we could test this?