From 2f917f37000a1ec72250aa5ec087df00f07e7538 Mon Sep 17 00:00:00 2001 From: Nikolay Kim Date: Wed, 20 Jun 2018 01:27:41 +0600 Subject: [PATCH] various cleanups and comments --- src/application.rs | 49 +++++++++++++++++++---------------- src/error.rs | 14 ++-------- src/header/shared/entity.rs | 2 +- src/header/shared/httpdate.rs | 6 +---- src/httpresponse.rs | 16 ++++++++++++ src/param.rs | 5 ++-- src/pipeline.rs | 44 +++++++++++++++++-------------- src/router.rs | 38 +++++++++++++-------------- src/scope.rs | 42 ++++++++++++++++++------------ src/ws/mask.rs | 46 ++++++++++++++------------------ 10 files changed, 137 insertions(+), 125 deletions(-) diff --git a/src/application.rs b/src/application.rs index 3c64e9254..93008b3d2 100644 --- a/src/application.rs +++ b/src/application.rs @@ -1,4 +1,4 @@ -use std::cell::{RefCell, UnsafeCell}; +use std::cell::RefCell; use std::collections::HashMap; use std::rc::Rc; @@ -21,7 +21,7 @@ pub struct HttpApplication { prefix: String, prefix_len: usize, router: Router, - inner: Rc>>, + inner: Rc>>, filters: Option>>>, middlewares: Rc>>>>, } @@ -69,17 +69,12 @@ impl PipelineHandler for Inner { } impl HttpApplication { - #[inline] - fn as_ref(&self) -> &Inner { - unsafe { &*self.inner.get() } - } - #[inline] fn get_handler(&self, req: &mut HttpRequest) -> HandlerType { if let Some(idx) = self.router.recognize(req) { HandlerType::Normal(idx) } else { - let inner = self.as_ref(); + let inner = self.inner.borrow(); req.match_info_mut().set_tail(0); 'outer: for idx in 0..inner.handlers.len() { @@ -131,7 +126,7 @@ impl HttpApplication { #[cfg(test)] pub(crate) fn run(&mut self, mut req: HttpRequest) -> AsyncResult { let tp = self.get_handler(&mut req); - unsafe { &mut *self.inner.get() }.handle(req, tp) + self.inner.borrow_mut().handle(req, tp) } #[cfg(test)] @@ -340,24 +335,32 @@ where T: FromRequest + 'static, { { - let parts: &mut ApplicationParts = unsafe { - &mut *(self.parts.as_mut().expect("Use after finish") as *mut _) - }; + let parts = self.parts.as_mut().expect("Use after finish"); // get resource handler - for &mut (ref pattern, ref mut handler) in &mut parts.resources { - if let Some(ref mut handler) = *handler { - if pattern.pattern() == path { - handler.method(method).with(f); - return self; - } + let mut found = false; + for &mut (ref pattern, ref handler) in &mut parts.resources { + if handler.is_some() && pattern.pattern() == path { + found = true; + break; } } - let mut handler = ResourceHandler::default(); - handler.method(method).with(f); - let pattern = Resource::new(handler.get_name(), path); - parts.resources.push((pattern, Some(handler))); + if !found { + let mut handler = ResourceHandler::default(); + handler.method(method).with(f); + let pattern = Resource::new(handler.get_name(), path); + parts.resources.push((pattern, Some(handler))); + } else { + for &mut (ref pattern, ref mut handler) in &mut parts.resources { + if let Some(ref mut handler) = *handler { + if pattern.pattern() == path { + handler.method(method).with(f); + break; + } + } + } + } } self } @@ -626,7 +629,7 @@ where let (router, resources) = Router::new(&prefix, parts.settings, resources); - let inner = Rc::new(UnsafeCell::new(Inner { + let inner = Rc::new(RefCell::new(Inner { prefix: prefix_len, default: parts.default, encoding: parts.encoding, diff --git a/src/error.rs b/src/error.rs index 395418d9e..f011733b3 100644 --- a/src/error.rs +++ b/src/error.rs @@ -3,7 +3,7 @@ use std::io::Error as IoError; use std::str::Utf8Error; use std::string::FromUtf8Error; use std::sync::Mutex; -use std::{fmt, io, mem, result}; +use std::{fmt, io, result}; use actix::MailboxError; use cookie; @@ -22,7 +22,6 @@ pub use url::ParseError as UrlParseError; // re-exports pub use cookie::ParseError as CookieParseError; -use body::Body; use handler::Responder; use httprequest::HttpRequest; use httpresponse::{HttpResponse, InnerHttpResponse}; @@ -671,16 +670,7 @@ impl InternalError { /// Create `InternalError` with predefined `HttpResponse`. pub fn from_response(cause: T, response: HttpResponse) -> Self { let mut resp = response.into_inner(); - let body = mem::replace(&mut resp.body, Body::Empty); - match body { - Body::Empty => (), - Body::Binary(mut bin) => { - resp.body = Body::Binary(bin.take().into()); - } - Body::Streaming(_) | Body::Actor(_) => { - error!("Streaming or Actor body is not support by error response"); - } - } + resp.drop_unsupported_body(); InternalError { cause, diff --git a/src/header/shared/entity.rs b/src/header/shared/entity.rs index a83ce1956..0d3b0a4ef 100644 --- a/src/header/shared/entity.rs +++ b/src/header/shared/entity.rs @@ -161,7 +161,7 @@ impl IntoHeaderValue for EntityTag { fn try_into(self) -> Result { let mut wrt = Writer::new(); write!(wrt, "{}", self).unwrap(); - unsafe { Ok(HeaderValue::from_shared_unchecked(wrt.take())) } + HeaderValue::from_shared(wrt.take()) } } diff --git a/src/header/shared/httpdate.rs b/src/header/shared/httpdate.rs index 60075e1a2..7fd26b121 100644 --- a/src/header/shared/httpdate.rs +++ b/src/header/shared/httpdate.rs @@ -64,11 +64,7 @@ impl IntoHeaderValue for HttpDate { fn try_into(self) -> Result { let mut wrt = BytesMut::with_capacity(29).writer(); write!(wrt, "{}", self.0.rfc822()).unwrap(); - unsafe { - Ok(HeaderValue::from_shared_unchecked( - wrt.get_mut().take().freeze(), - )) - } + HeaderValue::from_shared(wrt.get_mut().take().freeze()) } } diff --git a/src/httpresponse.rs b/src/httpresponse.rs index a0eb46a6c..8caf470ce 100644 --- a/src/httpresponse.rs +++ b/src/httpresponse.rs @@ -894,7 +894,9 @@ pub(crate) struct InnerHttpResponse { error: Option, } +/// This is here only because `failure::Fail: Send + Sync` which looks insane to me unsafe impl Sync for InnerHttpResponse {} +/// This is here only because `failure::Fail: Send + Sync` which looks insane to me unsafe impl Send for InnerHttpResponse {} impl InnerHttpResponse { @@ -914,6 +916,20 @@ impl InnerHttpResponse { error: None, } } + + /// This is for failure, we can not have Send + Sync on Streaming and Actor response + pub(crate) fn drop_unsupported_body(&mut self) { + let body = mem::replace(&mut self.body, Body::Empty); + match body { + Body::Empty => (), + Body::Binary(mut bin) => { + self.body = Body::Binary(bin.take().into()); + } + Body::Streaming(_) | Body::Actor(_) => { + error!("Streaming or Actor body is not support by error response"); + } + } + } } /// Internal use only! unsafe diff --git a/src/param.rs b/src/param.rs index 02c1aea8a..1329ff680 100644 --- a/src/param.rs +++ b/src/param.rs @@ -1,10 +1,11 @@ -use http::StatusCode; -use smallvec::SmallVec; use std; use std::ops::Index; use std::path::PathBuf; use std::str::FromStr; +use http::StatusCode; +use smallvec::SmallVec; + use error::{InternalError, ResponseError, UriSegmentError}; use uri::Url; diff --git a/src/pipeline.rs b/src/pipeline.rs index 91e2143ea..2e03c8f62 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -124,7 +124,7 @@ impl PipelineInfo { impl> Pipeline { pub fn new( req: HttpRequest, mws: Rc>>>>, - handler: Rc>, htype: HandlerType, + handler: Rc>, htype: HandlerType, ) -> Pipeline { let mut info = PipelineInfo { mws, @@ -133,7 +133,7 @@ impl> Pipeline { error: None, context: None, disconnected: None, - encoding: unsafe { &*handler.get() }.encoding(), + encoding: handler.borrow().encoding(), }; let state = StartMiddlewares::init(&mut info, handler, htype); @@ -171,13 +171,12 @@ impl> HttpHandlerTask for Pipeline { } fn poll_io(&mut self, io: &mut Writer) -> Poll { - let info: &mut PipelineInfo<_> = unsafe { &mut *(&mut self.0 as *mut _) }; + let mut state = mem::replace(&mut self.1, PipelineState::None); loop { - if self.1.is_response() { - let state = mem::replace(&mut self.1, PipelineState::None); + if state.is_response() { if let PipelineState::Response(st) = state { - match st.poll_io(io, info) { + match st.poll_io(io, &mut self.0) { Ok(state) => { self.1 = state; if let Some(error) = self.0.error.take() { @@ -193,7 +192,7 @@ impl> HttpHandlerTask for Pipeline { } } } - match self.1 { + match state { PipelineState::None => return Ok(Async::Ready(true)), PipelineState::Error => { return Err( @@ -203,27 +202,32 @@ impl> HttpHandlerTask for Pipeline { _ => (), } - match self.1.poll(info) { - Some(state) => self.1 = state, - None => return Ok(Async::NotReady), + match state.poll(&mut self.0) { + Some(st) => state = st, + None => { + return { + self.1 = state; + Ok(Async::NotReady) + } + } } } } fn poll_completed(&mut self) -> Poll<(), Error> { - let info: &mut PipelineInfo<_> = unsafe { &mut *(&mut self.0 as *mut _) }; - + let mut state = mem::replace(&mut self.1, PipelineState::None); loop { - match self.1 { + match state { PipelineState::None | PipelineState::Error => { return Ok(Async::Ready(())) } _ => (), } - if let Some(state) = self.1.poll(info) { - self.1 = state; + if let Some(st) = state.poll(&mut self.0) { + state = st; } else { + self.1 = state; return Ok(Async::NotReady); } } @@ -234,7 +238,7 @@ type Fut = Box, Error = Error>>; /// Middlewares start executor struct StartMiddlewares { - hnd: Rc>, + hnd: Rc>, htype: HandlerType, fut: Option, _s: PhantomData, @@ -242,14 +246,14 @@ struct StartMiddlewares { impl> StartMiddlewares { fn init( - info: &mut PipelineInfo, hnd: Rc>, htype: HandlerType, + info: &mut PipelineInfo, hnd: Rc>, htype: HandlerType, ) -> PipelineState { // execute middlewares, we need this stage because middlewares could be // non-async and we can move to next state immediately let len = info.mws.borrow().len() as u16; loop { if info.count == len { - let reply = unsafe { &mut *hnd.get() }.handle(info.req().clone(), htype); + let reply = hnd.borrow_mut().handle(info.req().clone(), htype); return WaitingResponse::init(info, reply); } else { let state = @@ -285,7 +289,9 @@ impl> StartMiddlewares { } loop { if info.count == len { - let reply = unsafe { &mut *self.hnd.get() } + let reply = self + .hnd + .borrow_mut() .handle(info.req().clone(), self.htype); return Some(WaitingResponse::init(info, reply)); } else { diff --git a/src/router.rs b/src/router.rs index 6592f64e7..0ae178089 100644 --- a/src/router.rs +++ b/src/router.rs @@ -1,9 +1,9 @@ use std::collections::HashMap; use std::hash::{Hash, Hasher}; -use std::mem; use std::rc::Rc; use regex::{escape, Regex}; +use smallvec::SmallVec; use error::UrlGenerationError; use httprequest::HttpRequest; @@ -264,9 +264,9 @@ impl Resource { pub fn match_with_params( &self, req: &mut HttpRequest, plen: usize, insert: bool, ) -> bool { - let mut segments: [ParamItem; 24] = unsafe { mem::uninitialized() }; + let mut segments: SmallVec<[ParamItem; 5]> = SmallVec::new(); - let (names, segments_len) = { + let names = { let path = &req.path()[plen..]; if insert { if path.is_empty() { @@ -282,7 +282,6 @@ impl Resource { PatternType::Static(ref s) => return s == path, PatternType::Dynamic(ref re, ref names, _) => { if let Some(captures) = re.captures(path) { - let mut idx = 0; let mut passed = false; for capture in captures.iter() { if let Some(ref m) = capture { @@ -290,14 +289,13 @@ impl Resource { passed = true; continue; } - segments[idx] = ParamItem::UrlSegment( + segments.push(ParamItem::UrlSegment( (plen + m.start()) as u16, (plen + m.end()) as u16, - ); - idx += 1; + )); } } - (names, idx) + names } else { return false; } @@ -309,9 +307,11 @@ impl Resource { let len = req.path().len(); let params = req.match_info_mut(); params.set_tail(len as u16); - for idx in 0..segments_len { + for (idx, segment) in segments.iter().enumerate() { + // reason: Router is part of App, which is unique per thread + // app is alive during whole life of tthread let name = unsafe { &*(names[idx].as_str() as *const _) }; - params.add(name, segments[idx]); + params.add(name, *segment); } true } @@ -320,9 +320,9 @@ impl Resource { pub fn match_prefix_with_params( &self, req: &mut HttpRequest, plen: usize, ) -> Option { - let mut segments: [ParamItem; 24] = unsafe { mem::uninitialized() }; + let mut segments: SmallVec<[ParamItem; 5]> = SmallVec::new(); - let (names, segments_len, tail_len) = { + let (names, tail_len) = { let path = &req.path()[plen..]; let path = if path.is_empty() { "/" } else { path }; @@ -334,7 +334,6 @@ impl Resource { }, PatternType::Dynamic(ref re, ref names, len) => { if let Some(captures) = re.captures(path) { - let mut idx = 0; let mut pos = 0; let mut passed = false; for capture in captures.iter() { @@ -344,15 +343,14 @@ impl Resource { continue; } - segments[idx] = ParamItem::UrlSegment( + segments.push(ParamItem::UrlSegment( (plen + m.start()) as u16, (plen + m.end()) as u16, - ); - idx += 1; + )); pos = m.end(); } } - (names, idx, pos + len) + (names, pos + len) } else { return None; } @@ -378,9 +376,11 @@ impl Resource { let params = req.match_info_mut(); params.set_tail(tail_len as u16); - for idx in 0..segments_len { + for (idx, segment) in segments.iter().enumerate() { + // reason: Router is part of App, which is unique per thread + // app is alive during whole life of tthread let name = unsafe { &*(names[idx].as_str() as *const _) }; - params.add(name, segments[idx]); + params.add(name, *segment); } Some(tail_len) } diff --git a/src/scope.rs b/src/scope.rs index e57db7632..6651992db 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -226,27 +226,35 @@ impl Scope { R: Responder + 'static, T: FromRequest + 'static, { - // get resource handler - let slf: &Scope = unsafe { &*(&self as *const _) }; - for &(ref pattern, ref resource) in slf.resources.iter() { + // check if we have resource handler + let mut found = false; + for &(ref pattern, _) in self.resources.iter() { if pattern.pattern() == path { - resource.borrow_mut().method(method).with(f); - return self; + found = true; + break; } } - let mut handler = ResourceHandler::default(); - handler.method(method).with(f); - let pattern = Resource::with_prefix( - handler.get_name(), - path, - if path.is_empty() { "" } else { "/" }, - false, - ); - Rc::get_mut(&mut self.resources) - .expect("Can not use after configuration") - .push((pattern, Rc::new(RefCell::new(handler)))); - + if found { + for &(ref pattern, ref resource) in self.resources.iter() { + if pattern.pattern() == path { + resource.borrow_mut().method(method).with(f); + break; + } + } + } else { + let mut handler = ResourceHandler::default(); + handler.method(method).with(f); + let pattern = Resource::with_prefix( + handler.get_name(), + path, + if path.is_empty() { "" } else { "/" }, + false, + ); + Rc::get_mut(&mut self.resources) + .expect("Can not use after configuration") + .push((pattern, Rc::new(RefCell::new(handler)))); + } self } diff --git a/src/ws/mask.rs b/src/ws/mask.rs index 18a906754..e99b950c8 100644 --- a/src/ws/mask.rs +++ b/src/ws/mask.rs @@ -7,7 +7,7 @@ use std::ptr::copy_nonoverlapping; /// Mask/unmask a frame. #[inline] pub fn apply_mask(buf: &mut [u8], mask: u32) { - apply_mask_fast32(buf, mask) + unsafe { apply_mask_fast32(buf, mask) } } /// A safe unoptimized mask application. @@ -20,9 +20,11 @@ fn apply_mask_fallback(buf: &mut [u8], mask: &[u8; 4]) { } /// Faster version of `apply_mask()` which operates on 8-byte blocks. +/// +/// unsafe because uses pointer math and bit operations for performance #[inline] #[cfg_attr(feature = "cargo-clippy", allow(cast_lossless))] -fn apply_mask_fast32(buf: &mut [u8], mask_u32: u32) { +unsafe fn apply_mask_fast32(buf: &mut [u8], mask_u32: u32) { let mut ptr = buf.as_mut_ptr(); let mut len = buf.len(); @@ -32,10 +34,8 @@ fn apply_mask_fast32(buf: &mut [u8], mask_u32: u32) { let n = if head > 4 { head - 4 } else { head }; let mask_u32 = if n > 0 { - unsafe { - xor_mem(ptr, mask_u32, n); - ptr = ptr.offset(head as isize); - } + xor_mem(ptr, mask_u32, n); + ptr = ptr.offset(head as isize); len -= n; if cfg!(target_endian = "big") { mask_u32.rotate_left(8 * n as u32) @@ -47,11 +47,9 @@ fn apply_mask_fast32(buf: &mut [u8], mask_u32: u32) { }; if head > 4 { - unsafe { - *(ptr as *mut u32) ^= mask_u32; - ptr = ptr.offset(4); - len -= 4; - } + *(ptr as *mut u32) ^= mask_u32; + ptr = ptr.offset(4); + len -= 4; } mask_u32 } else { @@ -68,27 +66,21 @@ fn apply_mask_fast32(buf: &mut [u8], mask_u32: u32) { mask_u64 = mask_u64 << 32 | mask_u32 as u64; while len >= 8 { - unsafe { - *(ptr as *mut u64) ^= mask_u64; - ptr = ptr.offset(8); - len -= 8; - } + *(ptr as *mut u64) ^= mask_u64; + ptr = ptr.offset(8); + len -= 8; } } while len >= 4 { - unsafe { - *(ptr as *mut u32) ^= mask_u32; - ptr = ptr.offset(4); - len -= 4; - } + *(ptr as *mut u32) ^= mask_u32; + ptr = ptr.offset(4); + len -= 4; } // Possible last block. if len > 0 { - unsafe { - xor_mem(ptr, mask_u32, len); - } + xor_mem(ptr, mask_u32, len); } } @@ -107,7 +99,7 @@ unsafe fn xor_mem(ptr: *mut u8, mask: u32, len: usize) { #[cfg(test)] mod tests { - use super::{apply_mask_fallback, apply_mask_fast32}; + use super::{apply_mask, apply_mask_fallback}; use std::ptr; #[test] @@ -126,7 +118,7 @@ mod tests { apply_mask_fallback(&mut masked, &mask); let mut masked_fast = unmasked.clone(); - apply_mask_fast32(&mut masked_fast, mask_u32); + apply_mask(&mut masked_fast, mask_u32); assert_eq!(masked, masked_fast); } @@ -137,7 +129,7 @@ mod tests { apply_mask_fallback(&mut masked[1..], &mask); let mut masked_fast = unmasked.clone(); - apply_mask_fast32(&mut masked_fast[1..], mask_u32); + apply_mask(&mut masked_fast[1..], mask_u32); assert_eq!(masked, masked_fast); }