From cf8c2ca95ec04121f04f928d0cea151f240d4c3a Mon Sep 17 00:00:00 2001 From: Nikolay Kim Date: Tue, 26 Dec 2017 09:00:45 -0800 Subject: [PATCH] refactor Handler trait, use mut self --- guide/src/qs_4.md | 75 ++++++++++++++++++++++++++++++++++++++++++++++ src/application.rs | 26 +++++++++------- src/channel.rs | 2 +- src/fs.rs | 2 +- src/h1.rs | 2 +- src/h2.rs | 2 +- src/handler.rs | 20 ++++++------- src/httpcodes.rs | 4 +-- src/httprequest.rs | 16 +++++----- src/pipeline.rs | 8 ++--- src/resource.rs | 4 +-- src/route.rs | 2 +- src/router.rs | 25 ++++++++-------- src/server.rs | 9 +++--- 14 files changed, 138 insertions(+), 59 deletions(-) diff --git a/guide/src/qs_4.md b/guide/src/qs_4.md index 910e83f88..9ef785023 100644 --- a/guide/src/qs_4.md +++ b/guide/src/qs_4.md @@ -39,6 +39,81 @@ fn index(req: HttpRequest) -> Box> { } ``` +Some notes on shared application state and handler state. If you noticed +*Handler* trait is generic over *S*, which defines application state type. So +application state is accessible from handler with `HttpRequest::state()` method. +But state is accessible as a read-only reference, if you need mutable access to state +you have to implement it yourself. On other hand handler can mutable access it's own state +as `handle` method takes mutable reference to *self*. Beware, actix creates multiple copies +of application state and handlers, unique for each thread, so if you run your +application in several threads actix will create same amount as number of threads +of application state objects and handler objects. + +Here is example of handler that stores number of processed requests: + +```rust +# extern crate actix; +# extern crate actix_web; +use actix_web::*; +use actix_web::dev::Handler; + +struct MyHandler(usize); + +impl Handler for MyHandler { + type Result = HttpResponse; + + /// Handle request + fn handle(&mut self, req: HttpRequest) -> Self::Result { + self.0 += 1; + httpcodes::HTTPOk.response() + } +} +# fn main() {} +``` + +This handler will work, but `self.0` value will be different depends on number of threads and +number of requests processed per thread. Proper implementation would use `Arc` and `AtomicUsize` + +```rust +# extern crate actix; +# extern crate actix_web; +use actix_web::*; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; + +struct MyHandler(Arc); + +impl Handler for MyHandler { + type Result = HttpResponse; + + /// Handle request + fn handle(&mut self, req: HttpRequest) -> Self::Result { + let num = self.0.load(Ordering::Relaxed) + 1; + self.0.store(num, Ordering::Relaxed); + httpcodes::HTTPOk.response() + } +} + +fn main() { + let sys = actix::System::new("example"); + + let inc = Arc::new(AtomicUsize::new(0)); + + HttpServer::new( + move || { + let cloned = inc.clone(); + Application::new() + .resource("/", move |r| r.h(MyHandler(cloned))) + }) + .bind("127.0.0.1:8088").unwrap() + .start(); + + println!("Started http server: 127.0.0.1:8088"); +# actix::Arbiter::system().send(actix::msgs::SystemExit(0)); + let _ = sys.run(); +} +``` + ## Response with custom type To return custom type directly from handler function type needs to implement `Responder` trait. diff --git a/src/application.rs b/src/application.rs index 381ecac9e..80b8173ce 100644 --- a/src/application.rs +++ b/src/application.rs @@ -15,7 +15,8 @@ pub struct HttpApplication { state: Rc, prefix: String, default: Resource, - router: Router, + router: Router, + resources: Vec>, middlewares: Rc>>>, } @@ -25,9 +26,9 @@ impl HttpApplication { req.with_state(Rc::clone(&self.state), self.router.clone()) } - pub(crate) fn run(&self, mut req: HttpRequest) -> Reply { - if let Some(h) = self.router.recognize(&mut req) { - h.handle(req.clone(), Some(&self.default)) + pub(crate) fn run(&mut self, mut req: HttpRequest) -> Reply { + if let Some(idx) = self.router.recognize(&mut req) { + self.resources[idx].handle(req.clone(), Some(&mut self.default)) } else { self.default.handle(req, None) } @@ -36,11 +37,12 @@ impl HttpApplication { impl HttpHandler for HttpApplication { - fn handle(&self, req: HttpRequest) -> Result, HttpRequest> { + fn handle(&mut self, req: HttpRequest) -> Result, HttpRequest> { if req.path().starts_with(&self.prefix) { let req = self.prepare_request(req); + // TODO: redesign run callback Ok(Box::new(Pipeline::new(req, Rc::clone(&self.middlewares), - &|req: HttpRequest| self.run(req)))) + &mut |req: HttpRequest| self.run(req)))) } else { Err(req) } @@ -264,11 +266,13 @@ impl Application where S: 'static { resources.insert(pattern, None); } + let (router, resources) = Router::new(prefix, resources); HttpApplication { state: Rc::new(parts.state), prefix: prefix.to_owned(), default: parts.default, - router: Router::new(prefix, resources), + router: router, + resources: resources, middlewares: Rc::new(parts.middlewares), } } @@ -314,7 +318,7 @@ mod tests { #[test] fn test_default_resource() { - let app = Application::new() + let mut app = Application::new() .resource("/test", |r| r.h(httpcodes::HTTPOk)) .finish(); @@ -330,7 +334,7 @@ mod tests { let resp = app.run(req); assert_eq!(resp.as_response().unwrap().status(), StatusCode::NOT_FOUND); - let app = Application::new() + let mut app = Application::new() .default_resource(|r| r.h(httpcodes::HTTPMethodNotAllowed)) .finish(); let req = HttpRequest::new( @@ -342,7 +346,7 @@ mod tests { #[test] fn test_unhandled_prefix() { - let app = Application::new() + let mut app = Application::new() .prefix("/test") .resource("/test", |r| r.h(httpcodes::HTTPOk)) .finish(); @@ -351,7 +355,7 @@ mod tests { #[test] fn test_state() { - let app = Application::with_state(10) + let mut app = Application::with_state(10) .resource("/", |r| r.h(httpcodes::HTTPOk)) .finish(); let req = HttpRequest::default().with_state(Rc::clone(&app.state), app.router.clone()); diff --git a/src/channel.rs b/src/channel.rs index 6503e82f8..9940a297d 100644 --- a/src/channel.rs +++ b/src/channel.rs @@ -18,7 +18,7 @@ use server::{ServerSettings, WorkerSettings}; pub trait HttpHandler: 'static { /// Handle request - fn handle(&self, req: HttpRequest) -> Result, HttpRequest>; + fn handle(&mut self, req: HttpRequest) -> Result, HttpRequest>; /// Set server settings fn server_settings(&mut self, settings: ServerSettings) {} diff --git a/src/fs.rs b/src/fs.rs index 78f3b4e72..c7dc68dc7 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -252,7 +252,7 @@ impl StaticFiles { impl Handler for StaticFiles { type Result = Result; - fn handle(&self, req: HttpRequest) -> Self::Result { + fn handle(&mut self, req: HttpRequest) -> Self::Result { if !self.accessible { Err(io::Error::new(io::ErrorKind::NotFound, "not found")) } else { diff --git a/src/h1.rs b/src/h1.rs index 95fbb8c40..3f23029b0 100644 --- a/src/h1.rs +++ b/src/h1.rs @@ -231,7 +231,7 @@ impl Http1 // start request processing let mut pipe = None; - for h in self.settings.handlers().iter() { + for h in self.settings.handlers().iter_mut() { req = match h.handle(req) { Ok(t) => { pipe = Some(t); diff --git a/src/h2.rs b/src/h2.rs index 5a3e81ac2..f0ac8cb77 100644 --- a/src/h2.rs +++ b/src/h2.rs @@ -261,7 +261,7 @@ impl Entry { // start request processing let mut task = None; - for h in settings.handlers().iter() { + for h in settings.handlers().iter_mut() { req = match h.handle(req) { Ok(t) => { task = Some(t); diff --git a/src/handler.rs b/src/handler.rs index f58513a95..a5e34223b 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -19,7 +19,7 @@ pub trait Handler: 'static { type Result: Responder; /// Handle request - fn handle(&self, req: HttpRequest) -> Self::Result; + fn handle(&mut self, req: HttpRequest) -> Self::Result; } /// Trait implemented by types that generate responses for clients. @@ -59,7 +59,7 @@ impl Handler for F { type Result = R; - fn handle(&self, req: HttpRequest) -> R { + fn handle(&mut self, req: HttpRequest) -> R { (self)(req) } } @@ -207,7 +207,7 @@ impl Responder for Box> /// Trait defines object that could be regestered as resource route pub(crate) trait RouteHandler: 'static { - fn handle(&self, req: HttpRequest) -> Reply; + fn handle(&mut self, req: HttpRequest) -> Reply; } /// Route handler wrapper for Handler @@ -236,7 +236,7 @@ impl RouteHandler for WrapHandler R: Responder + 'static, S: 'static, { - fn handle(&self, req: HttpRequest) -> Reply { + fn handle(&mut self, req: HttpRequest) -> Reply { let req2 = req.clone_without_state(); match self.h.handle(req).respond_to(req2) { Ok(reply) => reply.into(), @@ -277,7 +277,7 @@ impl RouteHandler for AsyncHandler E: Into + 'static, S: 'static, { - fn handle(&self, req: HttpRequest) -> Reply { + fn handle(&mut self, req: HttpRequest) -> Reply { let req2 = req.clone_without_state(); let fut = (self.h)(req) .map_err(|e| e.into()) @@ -368,7 +368,7 @@ impl NormalizePath { impl Handler for NormalizePath { type Result = Result; - fn handle(&self, req: HttpRequest) -> Self::Result { + fn handle(&mut self, req: HttpRequest) -> Self::Result { if let Some(router) = req.router() { let query = req.query_string(); if self.merge { @@ -420,7 +420,7 @@ mod tests { #[test] fn test_normalize_path_trailing_slashes() { - let app = Application::new() + let mut app = Application::new() .resource("/resource1", |r| r.method(Method::GET).f(index)) .resource("/resource2/", |r| r.method(Method::GET).f(index)) .default_resource(|r| r.h(NormalizePath::default())) @@ -452,7 +452,7 @@ mod tests { #[test] fn test_normalize_path_trailing_slashes_disabled() { - let app = Application::new() + let mut app = Application::new() .resource("/resource1", |r| r.method(Method::GET).f(index)) .resource("/resource2/", |r| r.method(Method::GET).f(index)) .default_resource(|r| r.h( @@ -479,7 +479,7 @@ mod tests { #[test] fn test_normalize_path_merge_slashes() { - let app = Application::new() + let mut app = Application::new() .resource("/resource1", |r| r.method(Method::GET).f(index)) .resource("/resource1/a/b", |r| r.method(Method::GET).f(index)) .default_resource(|r| r.h(NormalizePath::default())) @@ -515,7 +515,7 @@ mod tests { #[test] fn test_normalize_path_merge_and_append_slashes() { - let app = Application::new() + let mut app = Application::new() .resource("/resource1", |r| r.method(Method::GET).f(index)) .resource("/resource2/", |r| r.method(Method::GET).f(index)) .resource("/resource1/a/b", |r| r.method(Method::GET).f(index)) diff --git a/src/httpcodes.rs b/src/httpcodes.rs index 274167c99..abe226cbe 100644 --- a/src/httpcodes.rs +++ b/src/httpcodes.rs @@ -70,13 +70,13 @@ impl StaticResponse { impl Handler for StaticResponse { type Result = HttpResponse; - fn handle(&self, _: HttpRequest) -> HttpResponse { + fn handle(&mut self, _: HttpRequest) -> HttpResponse { HttpResponse::new(self.0, Body::Empty) } } impl RouteHandler for StaticResponse { - fn handle(&self, _: HttpRequest) -> Reply { + fn handle(&mut self, _: HttpRequest) -> Reply { Reply::response(HttpResponse::new(self.0, Body::Empty)) } } diff --git a/src/httprequest.rs b/src/httprequest.rs index 1f2e9eb05..cf79ab33d 100644 --- a/src/httprequest.rs +++ b/src/httprequest.rs @@ -87,7 +87,7 @@ impl HttpMessage { } /// An HTTP Request -pub struct HttpRequest(SharedHttpMessage, Option>, Option>); +pub struct HttpRequest(SharedHttpMessage, Option>, Option); impl HttpRequest<()> { /// Construct a new Request. @@ -146,7 +146,7 @@ impl HttpRequest<()> { #[inline] /// Construct new http request with state. - pub fn with_state(self, state: Rc, router: Router) -> HttpRequest { + pub fn with_state(self, state: Rc, router: Router) -> HttpRequest { HttpRequest(self.0, Some(state), Some(router)) } } @@ -277,7 +277,7 @@ impl HttpRequest { /// This method returns reference to current `Router` object. #[inline] - pub fn router(&self) -> Option<&Router> { + pub fn router(&self) -> Option<&Router> { self.2.as_ref() } @@ -736,11 +736,11 @@ mod tests { let mut req = HttpRequest::new(Method::GET, Uri::from_str("/value/?id=test").unwrap(), Version::HTTP_11, HeaderMap::new(), None); - let mut resource = Resource::default(); + let mut resource = Resource::<()>::default(); resource.name("index"); let mut map = HashMap::new(); map.insert(Pattern::new("index", "/{key}/"), Some(resource)); - let router = Router::new("", map); + let (router, _) = Router::new("", map); assert!(router.recognize(&mut req).is_some()); assert_eq!(req.match_info().get("key"), Some("value")); @@ -843,11 +843,11 @@ mod tests { let req = HttpRequest::new( Method::GET, Uri::from_str("/").unwrap(), Version::HTTP_11, headers, None); - let mut resource = Resource::default(); + let mut resource = Resource::<()>::default(); resource.name("index"); let mut map = HashMap::new(); map.insert(Pattern::new("index", "/user/{name}.{ext}"), Some(resource)); - let router = Router::new("", map); + let (router, _) = Router::new("", map); assert!(router.has_route("/user/test.html")); assert!(!router.has_route("/test/unknown")); @@ -874,7 +874,7 @@ mod tests { resource.name("index"); let mut map = HashMap::new(); map.insert(Pattern::new("youtube", "https://youtube.com/watch/{video_id}"), None); - let router = Router::new("", map); + let (router, _) = Router::new::<()>("", map); assert!(!router.has_route("https://youtube.com/watch/unknown")); let req = req.with_state(Rc::new(()), router); diff --git a/src/pipeline.rs b/src/pipeline.rs index 71b3e7aff..cc17d2f33 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -15,8 +15,8 @@ use httprequest::HttpRequest; use httpresponse::HttpResponse; use middlewares::{Middleware, Finished, Started, Response}; -type Handler = Fn(HttpRequest) -> Reply; -pub(crate) type PipelineHandler<'a, S> = &'a Fn(HttpRequest) -> Reply; +type Handler = FnMut(HttpRequest) -> Reply; +pub(crate) type PipelineHandler<'a, S> = &'a mut FnMut(HttpRequest) -> Reply; pub struct Pipeline(PipelineInfo, PipelineState); @@ -287,7 +287,7 @@ impl StartMiddlewares { let len = info.mws.len(); loop { if info.count == len { - let reply = (&*handler)(info.req.clone()); + let reply = (&mut *handler)(info.req.clone()); return WaitingResponse::init(info, reply) } else { match info.mws[info.count].start(&mut info.req) { @@ -329,7 +329,7 @@ impl StartMiddlewares { return Ok(RunMiddlewares::init(info, resp)); } if info.count == len { - let reply = (unsafe{&*self.hnd})(info.req.clone()); + let reply = (unsafe{&mut *self.hnd})(info.req.clone()); return Ok(WaitingResponse::init(info, reply)); } else { loop { diff --git a/src/resource.rs b/src/resource.rs index 937c28251..ee6d682e5 100644 --- a/src/resource.rs +++ b/src/resource.rs @@ -126,10 +126,10 @@ impl Resource { self.routes.last_mut().unwrap().f(handler) } - pub(crate) fn handle(&self, mut req: HttpRequest, default: Option<&Resource>) + pub(crate) fn handle(&mut self, mut req: HttpRequest, default: Option<&mut Resource>) -> Reply { - for route in &self.routes { + for route in &mut self.routes { if route.check(&mut req) { return route.handle(req) } diff --git a/src/route.rs b/src/route.rs index 404fa16d3..64b60603d 100644 --- a/src/route.rs +++ b/src/route.rs @@ -36,7 +36,7 @@ impl Route { true } - pub(crate) fn handle(&self, req: HttpRequest) -> Reply { + pub(crate) fn handle(&mut self, req: HttpRequest) -> Reply { self.handler.handle(req) } diff --git a/src/router.rs b/src/router.rs index b5cdd8330..7fd9f2a65 100644 --- a/src/router.rs +++ b/src/router.rs @@ -12,20 +12,20 @@ use server::ServerSettings; /// Interface for application router. -pub struct Router(Rc>); +pub struct Router(Rc); -struct Inner { +struct Inner { prefix: String, regset: RegexSet, named: HashMap, patterns: Vec, - resources: Vec>, srv: ServerSettings, } -impl Router { +impl Router { /// Create new router - pub fn new(prefix: &str, map: HashMap>>) -> Router + pub fn new(prefix: &str, map: HashMap>>) + -> (Router, Vec>) { let prefix = prefix.trim().trim_right_matches('/').to_owned(); let mut named = HashMap::new(); @@ -46,13 +46,12 @@ impl Router { } } - Router(Rc::new( + (Router(Rc::new( Inner{ prefix: prefix, regset: RegexSet::new(&paths).unwrap(), named: named, patterns: patterns, - resources: resources, - srv: ServerSettings::default() })) + srv: ServerSettings::default() })), resources) } pub(crate) fn set_server_settings(&mut self, settings: ServerSettings) { @@ -72,7 +71,7 @@ impl Router { } /// Query for matched resource - pub fn recognize(&self, req: &mut HttpRequest) -> Option<&Resource> { + pub fn recognize(&self, req: &mut HttpRequest) -> Option { let mut idx = None; { let path = &req.path()[self.0.prefix.len()..]; @@ -88,7 +87,7 @@ impl Router { if let Some(idx) = idx { let path: &str = unsafe{ mem::transmute(&req.path()[self.0.prefix.len()..]) }; self.0.patterns[idx].update_match_info(path, req); - return Some(&self.0.resources[idx]) + return Some(idx) } else { None } @@ -128,8 +127,8 @@ impl Router { } } -impl Clone for Router { - fn clone(&self) -> Router { +impl Clone for Router { + fn clone(&self) -> Router { Router(Rc::clone(&self.0)) } } @@ -315,7 +314,7 @@ mod tests { routes.insert(Pattern::new("", "/v{val}/{val2}/index.html"), Some(Resource::default())); routes.insert(Pattern::new("", "/v/{tail:.*}"), Some(Resource::default())); routes.insert(Pattern::new("", "{test}/index.html"), Some(Resource::default())); - let rec = Router::new("", routes); + let (rec, _) = Router::new::<()>("", routes); let mut req = HttpRequest::new( Method::GET, Uri::from_str("/name").unwrap(), diff --git a/src/server.rs b/src/server.rs index 08d6645c6..fcdad1e6f 100644 --- a/src/server.rs +++ b/src/server.rs @@ -1,5 +1,6 @@ use std::{io, net, thread}; use std::rc::Rc; +use std::cell::{RefCell, RefMut}; use std::sync::Arc; use std::time::Duration; use std::marker::PhantomData; @@ -447,7 +448,7 @@ struct Worker { } pub(crate) struct WorkerSettings { - h: Vec, + h: RefCell>, enabled: bool, keep_alive: u64, bytes: Rc, @@ -457,7 +458,7 @@ pub(crate) struct WorkerSettings { impl WorkerSettings { pub(crate) fn new(h: Vec, keep_alive: Option) -> WorkerSettings { WorkerSettings { - h: h, + h: RefCell::new(h), enabled: if let Some(ka) = keep_alive { ka > 0 } else { false }, keep_alive: keep_alive.unwrap_or(0), bytes: Rc::new(helpers::SharedBytesPool::new()), @@ -465,8 +466,8 @@ impl WorkerSettings { } } - pub fn handlers(&self) -> &Vec { - &self.h + pub fn handlers(&self) -> RefMut> { + self.h.borrow_mut() } pub fn keep_alive(&self) -> u64 { self.keep_alive