feat(service): change Service::call to take &self (#3223)

change Service::call to take &self instead of &mut self.
Because of this change, the trait bound in the service::util::service_fn and
the trait bound in the impl for the ServiceFn struct were changed from FnMut to Fn.
This change was decided on for the following reasons:
- It prepares the way for async fn,
  since then the future only borrows &self, and thus a Service can concurrently handle
  multiple outstanding requests at once.
- It's clearer that Services can likely be cloned
- To share state across clones you generally need Arc<Mutex<_>>
  that means you're not really using the &mut self and could do with a &self

Closes #3040

BREAKING CHANGE: The Service::call function no longer takes a mutable reference to self.
  The FnMut trait bound on the service::util::service_fn function and the trait bound
  on the impl for the ServiceFn struct were changed from FnMut to Fn.

Author: rob2244

examples/service_struct_impl.rs

@@ -8,6 +8,7 @@ use tokio::net::TcpListener;
 use std::future::Future;
 use std::net::SocketAddr;
 use std::pin::Pin;
+use std::sync::Mutex;
 
 type Counter = i32;
 
@@ -23,7 +24,12 @@ async fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
 
         tokio::task::spawn(async move {
             if let Err(err) = http1::Builder::new()
-                .serve_connection(stream, Svc { counter: 81818 })
+                .serve_connection(
+                    stream,
+                    Svc {
+                        counter: Mutex::new(81818),
+                    },
+                )
                 .await
             {
                 println!("Failed to serve connection: {:?}", err);
@@ -33,15 +39,15 @@ async fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
 }
 
 struct Svc {
-    counter: Counter,
+    counter: Mutex<Counter>,
 }
 
 impl Service<Request<IncomingBody>> for Svc {
     type Response = Response<Full<Bytes>>;
     type Error = hyper::Error;
     type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;
 
-    fn call(&mut self, req: Request<IncomingBody>) -> Self::Future {
+    fn call(&self, req: Request<IncomingBody>) -> Self::Future {
         fn mk_response(s: String) -> Result<Response<Full<Bytes>>, hyper::Error> {
             Ok(Response::builder().body(Full::new(Bytes::from(s))).unwrap())
         }
@@ -58,7 +64,7 @@ impl Service<Request<IncomingBody>> for Svc {
         };
 
         if req.uri().path() != "/favicon.ico" {
-            self.counter += 1;
+            *self.counter.lock().expect("lock poisoned") += 1;
         }
 
         Box::pin(async { res })

src/service/service.rs

@@ -28,5 +28,13 @@ pub trait Service<Request> {
     type Future: Future<Output = Result<Self::Response, Self::Error>>;
 
     /// Process the request and return the response asynchronously.
-    fn call(&mut self, req: Request) -> Self::Future;
+    /// call takes a &self instead of a mut &self because:
+    /// - It prepares the way for async fn,
+    ///   since then the future only borrows &self, and thus a Service can concurrently handle
+    ///   multiple outstanding requests at once.
+    /// - It's clearer that Services can likely be cloned
+    /// - To share state across clones you generally need Arc<Mutex<_>>
+    ///   that means you're not really using the &mut self and could do with a &self
+    /// To see the discussion on this see: https://github.com/hyperium/hyper/issues/3040
+    fn call(&self, req: Request) -> Self::Future;
 }

src/service/util.rs

@@ -29,7 +29,7 @@ use crate::{Request, Response};
 /// ```
 pub fn service_fn<F, R, S>(f: F) -> ServiceFn<F, R>
 where
-    F: FnMut(Request<R>) -> S,
+    F: Fn(Request<R>) -> S,
     S: Future,
 {
     ServiceFn {
@@ -46,7 +46,7 @@ pub struct ServiceFn<F, R> {
 
 impl<F, ReqBody, Ret, ResBody, E> Service<Request<ReqBody>> for ServiceFn<F, ReqBody>
 where
-    F: FnMut(Request<ReqBody>) -> Ret,
+    F: Fn(Request<ReqBody>) -> Ret,
     ReqBody: Body,
     Ret: Future<Output = Result<Response<ResBody>, E>>,
     E: Into<Box<dyn StdError + Send + Sync>>,
@@ -56,7 +56,7 @@ where
     type Error = E;
     type Future = Ret;
 
-    fn call(&mut self, req: Request<ReqBody>) -> Self::Future {
+    fn call(&self, req: Request<ReqBody>) -> Self::Future {
         (self.f)(req)
     }
 }

tests/server.rs

@@ -2656,7 +2656,7 @@ impl Service<Request<IncomingBody>> for TestService {
     type Error = BoxError;
     type Future = BoxFuture;
 
-    fn call(&mut self, mut req: Request<IncomingBody>) -> Self::Future {
+    fn call(&self, mut req: Request<IncomingBody>) -> Self::Future {
         let tx = self.tx.clone();
         let replies = self.reply.clone();
 
@@ -2722,7 +2722,7 @@ impl Service<Request<IncomingBody>> for HelloWorld {
     type Error = hyper::Error;
     type Future = future::Ready<Result<Self::Response, Self::Error>>;
 
-    fn call(&mut self, _req: Request<IncomingBody>) -> Self::Future {
+    fn call(&self, _req: Request<IncomingBody>) -> Self::Future {
         let response = Response::new(Full::new(HELLO.into()));
         future::ok(response)
     }