From 5d3207830790420468b1e4f88d6a607df9e3379d Mon Sep 17 00:00:00 2001 From: "Sergey \"Shnatsel\" Davidoff" Date: Sun, 19 Jul 2020 16:25:10 +0200 Subject: [PATCH] Remove the unsound custom Cell and replace it with Rc - the way it was before the introduction of a custom cell. Heavily based on the patch by Wim Looman https://gist.github.com/Nemo157/b495b84b5b2f3134d19ad71ba5002066 --- actix-service/src/and_then.rs | 17 ++++---- actix-service/src/and_then_apply_fn.rs | 22 +++++----- actix-service/src/apply_cfg.rs | 28 +++++++------ actix-service/src/cell.rs | 57 -------------------------- actix-service/src/lib.rs | 1 - actix-service/src/then.rs | 16 ++++---- 6 files changed, 43 insertions(+), 98 deletions(-) delete mode 100644 actix-service/src/cell.rs diff --git a/actix-service/src/and_then.rs b/actix-service/src/and_then.rs index 74b64b38..bf402e8f 100644 --- a/actix-service/src/and_then.rs +++ b/actix-service/src/and_then.rs @@ -1,16 +1,17 @@ use std::future::Future; use std::pin::Pin; use std::rc::Rc; +use std::cell::RefCell; use std::task::{Context, Poll}; use super::{Service, ServiceFactory}; -use crate::cell::Cell; + /// Service for the `and_then` combinator, chaining a computation onto the end /// of another service which completes successfully. /// /// This is created by the `ServiceExt::and_then` method. -pub(crate) struct AndThenService(Cell<(A, B)>); +pub(crate) struct AndThenService(Rc>); impl AndThenService { /// Create new `AndThen` combinator @@ -19,7 +20,7 @@ impl AndThenService { A: Service, B: Service, { - Self(Cell::new((a, b))) + Self(Rc::new(RefCell::new((a, b)))) } } @@ -40,7 +41,7 @@ where type Future = AndThenServiceResponse; fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - let srv = self.0.get_mut(); + let mut srv = self.0.borrow_mut(); let not_ready = !srv.0.poll_ready(cx)?.is_ready(); if !srv.1.poll_ready(cx)?.is_ready() || not_ready { Poll::Pending @@ -51,7 +52,7 @@ where fn call(&mut self, req: A::Request) -> Self::Future { AndThenServiceResponse { - state: State::A(self.0.get_mut().0.call(req), Some(self.0.clone())), + state: State::A(self.0.borrow_mut().0.call(req), Some(self.0.clone())), } } } @@ -72,7 +73,7 @@ where A: Service, B: Service, { - A(#[pin] A::Future, Option>), + A(#[pin] A::Future, Option>>), B(#[pin] B::Future), Empty, } @@ -90,9 +91,9 @@ where match this.state.as_mut().project() { StateProj::A(fut, b) => match fut.poll(cx)? { Poll::Ready(res) => { - let mut b = b.take().unwrap(); + let b = b.take().unwrap(); this.state.set(State::Empty); // drop fut A - let fut = b.get_mut().1.call(res); + let fut = b.borrow_mut().1.call(res); this.state.set(State::B(fut)); self.poll(cx) } diff --git a/actix-service/src/and_then_apply_fn.rs b/actix-service/src/and_then_apply_fn.rs index de0cfac9..105d7ed7 100644 --- a/actix-service/src/and_then_apply_fn.rs +++ b/actix-service/src/and_then_apply_fn.rs @@ -2,9 +2,9 @@ use std::future::Future; use std::marker::PhantomData; use std::pin::Pin; use std::rc::Rc; +use std::cell::RefCell; use std::task::{Context, Poll}; -use crate::cell::Cell; use crate::{Service, ServiceFactory}; /// `Apply` service combinator @@ -16,7 +16,7 @@ where Fut: Future>, Err: From + From, { - srv: Cell<(A, B, F)>, + srv: Rc>, r: PhantomData<(Fut, Res, Err)>, } @@ -31,7 +31,7 @@ where /// Create new `Apply` combinator pub(crate) fn new(a: A, b: B, f: F) -> Self { Self { - srv: Cell::new((a, b, f)), + srv: Rc::new(RefCell::new((a, b, f))), r: PhantomData, } } @@ -67,7 +67,7 @@ where type Future = AndThenApplyFnFuture; fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - let inner = self.srv.get_mut(); + let mut inner = self.srv.borrow_mut(); let not_ready = inner.0.poll_ready(cx)?.is_pending(); if inner.1.poll_ready(cx)?.is_pending() || not_ready { Poll::Pending @@ -77,7 +77,7 @@ where } fn call(&mut self, req: A::Request) -> Self::Future { - let fut = self.srv.get_mut().0.call(req); + let fut = self.srv.borrow_mut().0.call(req); AndThenApplyFnFuture { state: State::A(fut, Some(self.srv.clone())), } @@ -108,7 +108,7 @@ where Err: From, Err: From, { - A(#[pin] A::Future, Option>), + A(#[pin] A::Future, Option>>), B(#[pin] Fut), Empty, } @@ -129,10 +129,10 @@ where match this.state.as_mut().project() { StateProj::A(fut, b) => match fut.poll(cx)? { Poll::Ready(res) => { - let mut b = b.take().unwrap(); + let b = b.take().unwrap(); this.state.set(State::Empty); - let b = b.get_mut(); - let fut = (&mut b.2)(res, &mut b.1); + let (_, b, f) = &mut *b.borrow_mut(); + let fut = f(res, b); this.state.set(State::B(fut)); self.poll(cx) } @@ -255,11 +255,11 @@ where if this.a.is_some() && this.b.is_some() { Poll::Ready(Ok(AndThenApplyFn { - srv: Cell::new(( + srv: Rc::new(RefCell::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 cee7b8c7..1fa4f0ee 100644 --- a/actix-service/src/apply_cfg.rs +++ b/actix-service/src/apply_cfg.rs @@ -2,8 +2,9 @@ use std::future::Future; use std::marker::PhantomData; use std::pin::Pin; use std::task::{Context, Poll}; +use std::rc::Rc; +use std::cell::RefCell; -use crate::cell::Cell; use crate::{Service, ServiceFactory}; /// Convert `Fn(Config, &mut Service1) -> Future` fn to a service factory @@ -26,7 +27,7 @@ where S: Service, { ApplyConfigService { - srv: Cell::new((srv, f)), + srv: Rc::new(RefCell::new((srv, f))), _t: PhantomData, } } @@ -53,7 +54,7 @@ where S: Service, { ApplyConfigServiceFactory { - srv: Cell::new((factory, f)), + srv: Rc::new(RefCell::new((factory, f))), _t: PhantomData, } } @@ -66,7 +67,7 @@ where R: Future>, S: Service, { - srv: Cell<(T, F)>, + srv: Rc>, _t: PhantomData<(C, R, S)>, } @@ -102,10 +103,8 @@ where type Future = R; fn new_service(&self, cfg: C) -> Self::Future { - unsafe { - let srv = self.srv.get_mut_unsafe(); - (srv.1)(cfg, &mut srv.0) - } + let (t, f) = &mut *self.srv.borrow_mut(); + f(cfg, t) } } @@ -117,7 +116,7 @@ where R: Future>, S: Service, { - srv: Cell<(T, F)>, + srv: Rc>, _t: PhantomData<(C, R, S)>, } @@ -157,7 +156,7 @@ where ApplyConfigServiceFactoryResponse { cfg: Some(cfg), store: self.srv.clone(), - state: State::A(self.srv.get_ref().0.new_service(())), + state: State::A(self.srv.borrow().0.new_service(())), } } } @@ -172,7 +171,7 @@ where S: Service, { cfg: Option, - store: Cell<(T, F)>, + store: Rc>, #[pin] state: State, } @@ -213,8 +212,11 @@ where }, StateProj::B(srv) => match srv.poll_ready(cx)? { Poll::Ready(_) => { - let fut = (this.store.get_mut().1)(this.cfg.take().unwrap(), srv); - this.state.set(State::C(fut)); + { + let (_, f) = &mut *this.store.borrow_mut(); + let fut = f(this.cfg.take().unwrap(), srv); + this.state.set(State::C(fut)); + } self.poll(cx) } Poll::Pending => Poll::Pending, diff --git a/actix-service/src/cell.rs b/actix-service/src/cell.rs deleted file mode 100644 index 3bcac0be..00000000 --- a/actix-service/src/cell.rs +++ /dev/null @@ -1,57 +0,0 @@ -//! Custom cell impl, internal use only -use std::task::{Context, Poll}; -use std::{cell::UnsafeCell, fmt, rc::Rc}; - -pub(crate) struct Cell { - inner: Rc>, -} - -impl Clone for Cell { - fn clone(&self) -> Self { - Self { - inner: self.inner.clone(), - } - } -} - -impl fmt::Debug for Cell { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.inner.fmt(f) - } -} - -impl Cell { - pub(crate) fn new(inner: T) -> Self { - Self { - inner: Rc::new(UnsafeCell::new(inner)), - } - } - - pub(crate) fn get_ref(&self) -> &T { - unsafe { &*self.inner.as_ref().get() } - } - - pub(crate) fn get_mut(&mut self) -> &mut T { - unsafe { &mut *self.inner.as_ref().get() } - } - - #[allow(clippy::mut_from_ref)] - pub(crate) unsafe fn get_mut_unsafe(&self) -> &mut T { - &mut *self.inner.as_ref().get() - } -} - -impl crate::Service for Cell { - type Request = T::Request; - type Response = T::Response; - type Error = T::Error; - type Future = T::Future; - - fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - self.get_mut().poll_ready(cx) - } - - fn call(&mut self, req: Self::Request) -> Self::Future { - self.get_mut().call(req) - } -} diff --git a/actix-service/src/lib.rs b/actix-service/src/lib.rs index 90d4c790..68a5dbc6 100644 --- a/actix-service/src/lib.rs +++ b/actix-service/src/lib.rs @@ -12,7 +12,6 @@ mod and_then_apply_fn; mod apply; mod apply_cfg; pub mod boxed; -mod cell; mod fn_service; mod map; mod map_config; diff --git a/actix-service/src/then.rs b/actix-service/src/then.rs index 1286e742..8c7bfa85 100644 --- a/actix-service/src/then.rs +++ b/actix-service/src/then.rs @@ -1,16 +1,16 @@ use std::future::Future; use std::pin::Pin; use std::rc::Rc; +use std::cell::RefCell; use std::task::{Context, Poll}; use super::{Service, ServiceFactory}; -use crate::cell::Cell; /// Service for the `then` combinator, chaining a computation onto the end of /// another service. /// /// This is created by the `Pipeline::then` method. -pub(crate) struct ThenService(Cell<(A, B)>); +pub(crate) struct ThenService(Rc>); impl ThenService { /// Create new `.then()` combinator @@ -19,7 +19,7 @@ impl ThenService { A: Service, B: Service, Error = A::Error>, { - Self(Cell::new((a, b))) + Self(Rc::new(RefCell::new((a, b)))) } } @@ -40,7 +40,7 @@ where type Future = ThenServiceResponse; fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { - let srv = self.0.get_mut(); + let mut srv = self.0.borrow_mut(); let not_ready = !srv.0.poll_ready(cx)?.is_ready(); if !srv.1.poll_ready(cx)?.is_ready() || not_ready { Poll::Pending @@ -51,7 +51,7 @@ where fn call(&mut self, req: A::Request) -> Self::Future { ThenServiceResponse { - state: State::A(self.0.get_mut().0.call(req), Some(self.0.clone())), + state: State::A(self.0.borrow_mut().0.call(req), Some(self.0.clone())), } } } @@ -72,7 +72,7 @@ where A: Service, B: Service>, { - A(#[pin] A::Future, Option>), + A(#[pin] A::Future, Option>>), B(#[pin] B::Future), Empty, } @@ -90,9 +90,9 @@ where match this.state.as_mut().project() { StateProj::A(fut, b) => match fut.poll(cx) { Poll::Ready(res) => { - let mut b = b.take().unwrap(); + let b = b.take().unwrap(); this.state.set(State::Empty); // drop fut A - let fut = b.get_mut().1.call(res); + let fut = b.borrow_mut().1.call(res); this.state.set(State::B(fut)); self.poll(cx) }