From 81ef12a0fd0b982a43e120f2c0afc1b65772a189 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Wed, 19 Jan 2022 22:23:53 +0000 Subject: [PATCH] add warn log to from_parts if given request is cloned closes #2562 --- actix-web-codegen/src/route.rs | 14 +++++++------- src/request.rs | 4 ++++ src/service.rs | 32 +++++++++++++++++++++++++++++++- 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/actix-web-codegen/src/route.rs b/actix-web-codegen/src/route.rs index a4472efd2..cb1ba1ef6 100644 --- a/actix-web-codegen/src/route.rs +++ b/actix-web-codegen/src/route.rs @@ -302,13 +302,13 @@ impl ToTokens for Route { if methods.len() > 1 { quote! { .guard( - actix_web::guard::Any(actix_web::guard::#first()) - #(.or(actix_web::guard::#others()))* + ::actix_web::guard::Any(::actix_web::guard::#first()) + #(.or(::actix_web::guard::#others()))* ) } } else { quote! { - .guard(actix_web::guard::#first()) + .guard(::actix_web::guard::#first()) } } }; @@ -318,17 +318,17 @@ impl ToTokens for Route { #[allow(non_camel_case_types, missing_docs)] pub struct #name; - impl actix_web::dev::HttpServiceFactory for #name { + impl ::actix_web::dev::HttpServiceFactory for #name { fn register(self, __config: &mut actix_web::dev::AppService) { #ast - let __resource = actix_web::Resource::new(#path) + let __resource = ::actix_web::Resource::new(#path) .name(#resource_name) #method_guards - #(.guard(actix_web::guard::fn_guard(#guards)))* + #(.guard(::actix_web::guard::fn_guard(#guards)))* #(.wrap(#wrappers))* .#resource_type(#name); - actix_web::dev::HttpServiceFactory::register(__resource, __config) + ::actix_web::dev::HttpServiceFactory::register(__resource, __config) } } }; diff --git a/src/request.rs b/src/request.rs index d721f2ac7..63db56fd3 100644 --- a/src/request.rs +++ b/src/request.rs @@ -138,6 +138,10 @@ impl HttpRequest { &self.inner.path } + /// Returns a mutable reference to the URL parameters container. + /// + /// # Panics + /// Panics if this `HttpRequest` has been cloned. #[inline] pub(crate) fn match_info_mut(&mut self) -> &mut Path { &mut Rc::get_mut(&mut self.inner).unwrap().path diff --git a/src/service.rs b/src/service.rs index 162c90ec8..81424a654 100644 --- a/src/service.rs +++ b/src/service.rs @@ -97,6 +97,11 @@ impl ServiceRequest { /// Construct request from parts. pub fn from_parts(req: HttpRequest, payload: Payload) -> Self { + #[cfg(debug_assertions)] + if Rc::strong_count(&req.inner) > 1 { + log::warn!("Cloning an `HttpRequest` might cause panics."); + } + Self { req, payload } } @@ -663,7 +668,7 @@ service_tuple! { A B C D E F G H I J K L } #[cfg(test)] mod tests { use super::*; - use crate::test::{init_service, TestRequest}; + use crate::test::{self, init_service, TestRequest}; use crate::{guard, http, web, App, HttpResponse}; use actix_service::Service; use actix_utils::future::ok; @@ -810,4 +815,29 @@ mod tests { let resp = srv.call(req).await.unwrap(); assert_eq!(resp.status(), http::StatusCode::OK); } + + #[actix_rt::test] + #[should_panic(expected = "called `Option::unwrap()` on a `None` value")] + async fn cloning_request_panics() { + async fn index(_name: web::Path<(String,)>) -> &'static str { + "" + } + + let app = test::init_service( + App::new() + .wrap_fn(|req, svc| { + let (req, pl) = req.into_parts(); + let _req2 = req.clone(); + let req = ServiceRequest::from_parts(req, pl); + svc.call(req) + }) + .service( + web::resource("/resource1/{name}/index.html").route(web::get().to(index)), + ), + ) + .await; + + let req = test::TestRequest::default().to_request(); + let _res = test::call_service(&app, req).await; + } }