diff --git a/actix-service/CHANGES.md b/actix-service/CHANGES.md index 0dcc6103..137e402b 100644 --- a/actix-service/CHANGES.md +++ b/actix-service/CHANGES.md @@ -1,5 +1,11 @@ # Changes +## [1.0.5] - 2020-01-16 + +### Fixed + +* Fixed unsoundness in .and_then()/.then() service combinators + ## [1.0.4] - 2020-01-15 ### Fixed diff --git a/actix-service/Cargo.toml b/actix-service/Cargo.toml index 045585a8..1825a6e6 100644 --- a/actix-service/Cargo.toml +++ b/actix-service/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "actix-service" -version = "1.0.4" +version = "1.0.5" authors = ["Nikolay Kim "] description = "Actix service" keywords = ["network", "framework", "async", "futures"] diff --git a/actix-service/src/and_then.rs b/actix-service/src/and_then.rs index 07ddc01a..b67de0a0 100644 --- a/actix-service/src/and_then.rs +++ b/actix-service/src/and_then.rs @@ -1,5 +1,6 @@ use std::future::Future; use std::pin::Pin; +use std::rc::Rc; use std::task::{Context, Poll}; use super::{Service, ServiceFactory}; @@ -9,7 +10,7 @@ use crate::cell::Cell; /// of another service which completes successfully. /// /// This is created by the `ServiceExt::and_then` method. -pub struct AndThenService(Cell, Cell); +pub(crate) struct AndThenService(Cell<(A, B)>); impl AndThenService { /// Create new `AndThen` combinator @@ -18,13 +19,13 @@ impl AndThenService { A: Service, B: Service, { - Self(Cell::new(a), Cell::new(b)) + Self(Cell::new((a, b))) } } impl Clone for AndThenService { fn clone(&self) -> Self { - AndThenService(self.0.clone(), self.1.clone()) + AndThenService(self.0.clone()) } } @@ -39,8 +40,9 @@ where type Future = AndThenServiceResponse; fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - let not_ready = { !self.0.get_mut().poll_ready(cx)?.is_ready() }; - if !self.1.poll_ready(cx)?.is_ready() || not_ready { + let srv = self.0.get_mut(); + let not_ready = !srv.0.poll_ready(cx)?.is_ready(); + if !srv.1.poll_ready(cx)?.is_ready() || not_ready { Poll::Pending } else { Poll::Ready(Ok(())) @@ -49,13 +51,13 @@ where fn call(&mut self, req: A::Request) -> Self::Future { AndThenServiceResponse { - state: State::A(self.0.get_mut().call(req), Some(self.1.clone())), + state: State::A(self.0.get_mut().0.call(req), Some(self.0.clone())), } } } #[pin_project::pin_project] -pub struct AndThenServiceResponse +pub(crate) struct AndThenServiceResponse where A: Service, B: Service, @@ -70,7 +72,7 @@ where A: Service, B: Service, { - A(#[pin] A::Future, Option>), + A(#[pin] A::Future, Option>), B(#[pin] B::Future), Empty, } @@ -92,7 +94,7 @@ where Poll::Ready(res) => { let mut b = b.take().unwrap(); this.state.set(State::Empty); // drop fut A - let fut = b.get_mut().call(res); + let fut = b.get_mut().1.call(res); this.state.set(State::B(fut)); self.poll(cx) } @@ -108,7 +110,7 @@ where } /// `.and_then()` service factory combinator -pub struct AndThenServiceFactory +pub(crate) struct AndThenServiceFactory where A: ServiceFactory, A::Config: Clone, @@ -119,8 +121,7 @@ where InitError = A::InitError, >, { - a: A, - b: B, + inner: Rc<(A, B)>, } impl AndThenServiceFactory @@ -136,7 +137,9 @@ where { /// Create new `AndThenFactory` combinator pub(crate) fn new(a: A, b: B) -> Self { - Self { a, b } + Self { + inner: Rc::new((a, b)), + } } } @@ -161,34 +164,34 @@ where type Future = AndThenServiceFactoryResponse; fn new_service(&self, cfg: A::Config) -> Self::Future { + let inner = &*self.inner; AndThenServiceFactoryResponse::new( - self.a.new_service(cfg.clone()), - self.b.new_service(cfg), + inner.0.new_service(cfg.clone()), + inner.1.new_service(cfg), ) } } impl Clone for AndThenServiceFactory where - A: ServiceFactory + Clone, + A: ServiceFactory, A::Config: Clone, B: ServiceFactory< - Config = A::Config, - Request = A::Response, - Error = A::Error, - InitError = A::InitError, - > + Clone, + Config = A::Config, + Request = A::Response, + Error = A::Error, + InitError = A::InitError, + >, { fn clone(&self) -> Self { Self { - a: self.a.clone(), - b: self.b.clone(), + inner: self.inner.clone(), } } } #[pin_project::pin_project] -pub struct AndThenServiceFactoryResponse +pub(crate) struct AndThenServiceFactoryResponse where A: ServiceFactory, B: ServiceFactory, diff --git a/actix-service/src/and_then_apply_fn.rs b/actix-service/src/and_then_apply_fn.rs index b0aba072..4ff3b4d0 100644 --- a/actix-service/src/and_then_apply_fn.rs +++ b/actix-service/src/and_then_apply_fn.rs @@ -1,13 +1,14 @@ use std::future::Future; use std::marker::PhantomData; use std::pin::Pin; +use std::rc::Rc; use std::task::{Context, Poll}; use crate::cell::Cell; use crate::{Service, ServiceFactory}; /// `Apply` service combinator -pub struct AndThenApplyFn +pub(crate) struct AndThenApplyFn where A: Service, B: Service, @@ -15,8 +16,7 @@ where Fut: Future>, Err: From + From, { - a: A, - b: Cell<(B, F)>, + srv: Cell<(A, B, F)>, r: PhantomData<(Fut, Res, Err)>, } @@ -31,8 +31,7 @@ where /// Create new `Apply` combinator pub(crate) fn new(a: A, b: B, f: F) -> Self { Self { - a, - b: Cell::new((b, f)), + srv: Cell::new((a, b, f)), r: PhantomData, } } @@ -40,7 +39,7 @@ where impl Clone for AndThenApplyFn where - A: Service + Clone, + A: Service, B: Service, F: FnMut(A::Response, &mut B) -> Fut, Fut: Future>, @@ -48,8 +47,7 @@ where { fn clone(&self) -> Self { AndThenApplyFn { - a: self.a.clone(), - b: self.b.clone(), + srv: self.srv.clone(), r: PhantomData, } } @@ -69,8 +67,9 @@ where type Future = AndThenApplyFnFuture; fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - let not_ready = self.a.poll_ready(cx)?.is_pending(); - if self.b.get_mut().0.poll_ready(cx)?.is_pending() || not_ready { + let inner = self.srv.get_mut(); + let not_ready = inner.0.poll_ready(cx)?.is_pending(); + if inner.1.poll_ready(cx)?.is_pending() || not_ready { Poll::Pending } else { Poll::Ready(Ok(())) @@ -78,14 +77,15 @@ where } fn call(&mut self, req: A::Request) -> Self::Future { + let fut = self.srv.get_mut().0.call(req); AndThenApplyFnFuture { - state: State::A(self.a.call(req), Some(self.b.clone())), + state: State::A(fut, Some(self.srv.clone())), } } } #[pin_project::pin_project] -pub struct AndThenApplyFnFuture +pub(crate) struct AndThenApplyFnFuture where A: Service, B: Service, @@ -108,7 +108,7 @@ where Err: From, Err: From, { - A(#[pin] A::Future, Option>), + A(#[pin] A::Future, Option>), B(#[pin] Fut), Empty, } @@ -134,7 +134,7 @@ where let mut b = b.take().unwrap(); this.state.set(State::Empty); let b = b.get_mut(); - let fut = (&mut b.1)(res, &mut b.0); + let fut = (&mut b.2)(res, &mut b.1); this.state.set(State::B(fut)); self.poll(cx) } @@ -150,10 +150,8 @@ where } /// `AndThenApplyFn` service factory -pub struct AndThenApplyFnFactory { - a: A, - b: B, - f: F, +pub(crate) struct AndThenApplyFnFactory { + srv: Rc<(A, B, F)>, r: PhantomData<(Fut, Res, Err)>, } @@ -168,25 +166,16 @@ where /// Create new `ApplyNewService` new service instance pub(crate) fn new(a: A, b: B, f: F) -> Self { Self { - a: a, - b: b, - f: f, + srv: Rc::new((a, b, f)), r: PhantomData, } } } -impl Clone for AndThenApplyFnFactory -where - A: Clone, - B: Clone, - F: Clone, -{ +impl Clone for AndThenApplyFnFactory { fn clone(&self) -> Self { Self { - a: self.a.clone(), - b: self.b.clone(), - f: self.f.clone(), + srv: self.srv.clone(), r: PhantomData, } } @@ -210,18 +199,19 @@ where type Future = AndThenApplyFnFactoryResponse; fn new_service(&self, cfg: A::Config) -> Self::Future { + let srv = &*self.srv; AndThenApplyFnFactoryResponse { a: None, b: None, - f: self.f.clone(), - fut_a: self.a.new_service(cfg.clone()), - fut_b: self.b.new_service(cfg), + f: srv.2.clone(), + fut_a: srv.0.new_service(cfg.clone()), + fut_b: srv.1.new_service(cfg), } } } #[pin_project::pin_project] -pub struct AndThenApplyFnFactoryResponse +pub(crate) struct AndThenApplyFnFactoryResponse where A: ServiceFactory, B: ServiceFactory, @@ -267,8 +257,11 @@ where if this.a.is_some() && this.b.is_some() { Poll::Ready(Ok(AndThenApplyFn { - a: this.a.take().unwrap(), - b: Cell::new((this.b.take().unwrap(), this.f.clone())), + srv: Cell::new(( + this.a.take().unwrap(), + this.b.take().unwrap(), + this.f.clone(), + )), r: PhantomData, })) } else { diff --git a/actix-service/src/apply_cfg.rs b/actix-service/src/apply_cfg.rs index a295639b..5c69b813 100644 --- a/actix-service/src/apply_cfg.rs +++ b/actix-service/src/apply_cfg.rs @@ -7,7 +7,18 @@ use crate::cell::Cell; use crate::{Service, ServiceFactory}; /// Convert `Fn(Config, &mut Service1) -> Future` fn to a service factory -pub fn apply_cfg(srv: T, f: F) -> ApplyConfigService +pub fn apply_cfg( + srv: T, + f: F, +) -> impl ServiceFactory< + Config = C, + Request = S::Request, + Response = S::Response, + Error = S::Error, + Service = S, + InitError = E, + Future = R, +> + Clone where F: FnMut(C, &mut T) -> R, T: Service, @@ -26,7 +37,14 @@ where pub fn apply_cfg_factory( factory: T, f: F, -) -> ApplyConfigServiceFactory +) -> impl ServiceFactory< + Config = C, + Request = S::Request, + Response = S::Response, + Error = S::Error, + Service = S, + InitError = T::InitError, +> + Clone where F: FnMut(C, &mut T::Service) -> R, T: ServiceFactory, @@ -41,7 +59,7 @@ where } /// Convert `Fn(Config, &mut Server) -> Future` fn to NewService\ -pub struct ApplyConfigService +struct ApplyConfigService where F: FnMut(C, &mut T) -> R, T: Service, @@ -92,7 +110,7 @@ where } /// Convert `Fn(&Config) -> Future` fn to NewService -pub struct ApplyConfigServiceFactory +struct ApplyConfigServiceFactory where F: FnMut(C, &mut T::Service) -> R, T: ServiceFactory, @@ -145,7 +163,7 @@ where } #[pin_project::pin_project] -pub struct ApplyConfigServiceFactoryResponse +struct ApplyConfigServiceFactoryResponse where F: FnMut(C, &mut T::Service) -> R, T: ServiceFactory, diff --git a/actix-service/src/lib.rs b/actix-service/src/lib.rs index d018e0e4..90d4c790 100644 --- a/actix-service/src/lib.rs +++ b/actix-service/src/lib.rs @@ -361,10 +361,7 @@ where } pub mod dev { - pub use crate::and_then::{AndThenService, AndThenServiceFactory}; - pub use crate::and_then_apply_fn::{AndThenApplyFn, AndThenApplyFnFactory}; pub use crate::apply::{Apply, ApplyServiceFactory}; - pub use crate::apply_cfg::{ApplyConfigService, ApplyConfigServiceFactory}; pub use crate::fn_service::{ FnService, FnServiceConfig, FnServiceFactory, FnServiceNoConfig, }; @@ -372,7 +369,6 @@ pub mod dev { pub use crate::map_config::{MapConfig, UnitConfig}; pub use crate::map_err::{MapErr, MapErrServiceFactory}; pub use crate::map_init_err::MapInitErr; - pub use crate::then::{ThenService, ThenServiceFactory}; pub use crate::transform::ApplyTransform; pub use crate::transform_err::TransformMapInitErr; } diff --git a/actix-service/src/pipeline.rs b/actix-service/src/pipeline.rs index d6107ce4..69881e4b 100644 --- a/actix-service/src/pipeline.rs +++ b/actix-service/src/pipeline.rs @@ -46,7 +46,12 @@ impl Pipeline { /// /// Note that this function consumes the receiving service and returns a /// wrapped version of it. - pub fn and_then(self, service: F) -> Pipeline> + pub fn and_then( + self, + service: F, + ) -> Pipeline< + impl Service + Clone, + > where Self: Sized, F: IntoService, @@ -65,7 +70,7 @@ impl Pipeline { self, service: I, f: F, - ) -> Pipeline> + ) -> Pipeline + Clone> where Self: Sized, I: IntoService, @@ -84,7 +89,12 @@ impl Pipeline { /// /// Note that this function consumes the receiving pipeline and returns a /// wrapped version of it. - pub fn then(self, service: F) -> Pipeline> + pub fn then( + self, + service: F, + ) -> Pipeline< + impl Service + Clone, + > where Self: Sized, F: IntoService, @@ -168,7 +178,23 @@ pub struct PipelineFactory { impl PipelineFactory { /// Call another service after call to this one has resolved successfully. - pub fn and_then(self, factory: F) -> PipelineFactory> + pub fn and_then( + self, + factory: F, + ) -> PipelineFactory< + impl ServiceFactory< + Request = T::Request, + Response = U::Response, + Error = T::Error, + Config = T::Config, + InitError = T::InitError, + Service = impl Service< + Request = T::Request, + Response = U::Response, + Error = T::Error, + > + Clone, + > + Clone, + > where Self: Sized, T::Config: Clone, @@ -193,7 +219,16 @@ impl PipelineFactory { self, factory: I, f: F, - ) -> PipelineFactory> + ) -> PipelineFactory< + impl ServiceFactory< + Request = T::Request, + Response = Res, + Error = Err, + Config = T::Config, + InitError = T::InitError, + Service = impl Service + Clone, + > + Clone, + > where Self: Sized, T::Config: Clone, @@ -214,7 +249,23 @@ impl PipelineFactory { /// /// Note that this function consumes the receiving pipeline and returns a /// wrapped version of it. - pub fn then(self, factory: F) -> PipelineFactory> + pub fn then( + self, + factory: F, + ) -> PipelineFactory< + impl ServiceFactory< + Request = T::Request, + Response = U::Response, + Error = T::Error, + Config = T::Config, + InitError = T::InitError, + Service = impl Service< + Request = T::Request, + Response = U::Response, + Error = T::Error, + > + Clone, + > + Clone, + > where Self: Sized, T::Config: Clone, diff --git a/actix-service/src/then.rs b/actix-service/src/then.rs index 7cd59870..b3a624ab 100644 --- a/actix-service/src/then.rs +++ b/actix-service/src/then.rs @@ -1,5 +1,6 @@ use std::future::Future; use std::pin::Pin; +use std::rc::Rc; use std::task::{Context, Poll}; use super::{Service, ServiceFactory}; @@ -8,11 +9,8 @@ use crate::cell::Cell; /// Service for the `then` combinator, chaining a computation onto the end of /// another service. /// -/// This is created by the `ServiceExt::then` method. -pub struct ThenService { - a: A, - b: Cell, -} +/// This is created by the `Pipeline::then` method. +pub(crate) struct ThenService(Cell<(A, B)>); impl ThenService { /// Create new `.then()` combinator @@ -21,19 +19,13 @@ impl ThenService { A: Service, B: Service, Error = A::Error>, { - Self { a, b: Cell::new(b) } + Self(Cell::new((a, b))) } } -impl Clone for ThenService -where - A: Clone, -{ +impl Clone for ThenService { fn clone(&self) -> Self { - ThenService { - a: self.a.clone(), - b: self.b.clone(), - } + ThenService(self.0.clone()) } } @@ -47,9 +39,10 @@ where type Error = B::Error; type Future = ThenServiceResponse; - fn poll_ready(&mut self, ctx: &mut Context<'_>) -> Poll> { - let not_ready = !self.a.poll_ready(ctx)?.is_ready(); - if !self.b.get_mut().poll_ready(ctx)?.is_ready() || not_ready { + fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { + let srv = self.0.get_mut(); + let not_ready = !srv.0.poll_ready(cx)?.is_ready(); + if !srv.1.poll_ready(cx)?.is_ready() || not_ready { Poll::Pending } else { Poll::Ready(Ok(())) @@ -58,13 +51,13 @@ where fn call(&mut self, req: A::Request) -> Self::Future { ThenServiceResponse { - state: State::A(self.a.call(req), Some(self.b.clone())), + state: State::A(self.0.get_mut().0.call(req), Some(self.0.clone())), } } } #[pin_project::pin_project] -pub struct ThenServiceResponse +pub(crate) struct ThenServiceResponse where A: Service, B: Service>, @@ -79,7 +72,7 @@ where A: Service, B: Service>, { - A(#[pin] A::Future, Option>), + A(#[pin] A::Future, Option>), B(#[pin] B::Future), Empty, } @@ -101,7 +94,7 @@ where Poll::Ready(res) => { let mut b = b.take().unwrap(); this.state.set(State::Empty); // drop fut A - let fut = b.get_mut().call(res); + let fut = b.get_mut().1.call(res); this.state.set(State::B(fut)); self.poll(cx) } @@ -117,10 +110,7 @@ where } /// `.then()` service factory combinator -pub struct ThenServiceFactory { - a: A, - b: B, -} +pub(crate) struct ThenServiceFactory(Rc<(A, B)>); impl ThenServiceFactory where @@ -135,7 +125,7 @@ where { /// Create new `AndThen` combinator pub(crate) fn new(a: A, b: B) -> Self { - Self { a, b } + Self(Rc::new((a, b))) } } @@ -160,28 +150,19 @@ where type Future = ThenServiceFactoryResponse; fn new_service(&self, cfg: A::Config) -> Self::Future { - ThenServiceFactoryResponse::new( - self.a.new_service(cfg.clone()), - self.b.new_service(cfg), - ) + let srv = &*self.0; + ThenServiceFactoryResponse::new(srv.0.new_service(cfg.clone()), srv.1.new_service(cfg)) } } -impl Clone for ThenServiceFactory -where - A: Clone, - B: Clone, -{ +impl Clone for ThenServiceFactory { fn clone(&self) -> Self { - Self { - a: self.a.clone(), - b: self.b.clone(), - } + Self(self.0.clone()) } } #[pin_project::pin_project] -pub struct ThenServiceFactoryResponse +pub(crate) struct ThenServiceFactoryResponse where A: ServiceFactory, B: ServiceFactory<