Repository: tower-rs/tower (rust)

Issue:
`buffer::Service` should probably erase the inner service type
Currently, `tower::buffer::Service` is parameterized over the type of the inner `Service` inside the buffer worker. However, the `Service` struct does not actually need to know the type of the inner service, only the types of the inner service's `Future` and `Error` associated types. 

In many cases, the `Service` type will likely be a significantly longer type than the `Future` and `Error` types --- when the service is composed of a number of chained middleware services, its type may be quite complex. Although futures will also often be composed of many nested future types, these will generally not be quite as large as `Service` types, for a few reasons:
-  Some middleware `Service`s may not wrap the inner service's future, and simply perform logic in `call` and/or `poll_ready`. These `Service`s will be generic over the inner `Service` type, but will not wrap the `Future` type
- Some middleware may return boxed futures, erasing the inner `Future` type while still being generic over the inner `Service` type

Therefore, we may wish to consider changing the definition of the `buffer::Service` type to be generic over only the `Future` and `Error` types of the inner service. This could reduce type complexity that may impact compilation time (as well as binary size, if type names are ever displayed in the program). Of course, because this changes the type signature of `buffer::Worker`, this will have to wait until the next breaking change.

cc @olix0r, who initially suggested this.

Pull request:
buffer: change `Buffer` to be generic over inner service's `Future`
This commit changes the `Buffer` service to be generic over the inner
service's `Future` type, rather than over the `Service` type itself. The
`Worker` type is still generic over the inner `Service` (and, it must
be, as it stores an instance of that service). This should reduce type
complexity of buffers a bit, as they will erase nested service types.

Unfortunately, `Buffer` is still generic over the inner service's
`Future`, which may also be nested, but it is likely to be less complex
than the `Service`. It would be nice if we could erase the `Future` type
as well, but I don't believe this is possible without either boxing the
futures or changing them to always be spawned on a background task,
neither of which seems desirable here.

Closes #641

Patch:
diff --git a/tower/src/buffer/layer.rs b/tower/src/buffer/layer.rs
index aadc938..8ef9167 100644
--- a/tower/src/buffer/layer.rs
+++ b/tower/src/buffer/layer.rs
@@ -48,7 +48,7 @@ where
     S::Error: Into<crate::BoxError> + Send + Sync,
     Request: Send + 'static,
 {
-    type Service = Buffer<S, Request>;
+    type Service = Buffer<Request, S::Future>;
 
     fn layer(&self, service: S) -> Self::Service {
         Buffer::new(service, self.bound)
diff --git a/tower/src/buffer/service.rs b/tower/src/buffer/service.rs
index c685984..9493f10 100644
--- a/tower/src/buffer/service.rs
+++ b/tower/src/buffer/service.rs
@@ -4,7 +4,10 @@ use super::{
     worker::{Handle, Worker},
 };
 
-use std::task::{Context, Poll};
+use std::{
+    future::Future,
+    task::{Context, Poll},
+};
 use tokio::sync::{mpsc, oneshot};
 use tokio_util::sync::PollSender;
 use tower_service::Service;
@@ -13,18 +16,14 @@ use tower_service::Service;
 ///
 /// See the module documentation for more details.
 #[derive(Debug)]
-pub struct Buffer<T, Request>
-where
-    T: Service<Request>,
-{
-    tx: PollSender<Message<Request, T::Future>>,
+pub struct Buffer<Req, F> {
+    tx: PollSender<Message<Req, F>>,
     handle: Handle,
 }
 
-impl<T, Request> Buffer<T, Request>
+impl<Req, F> Buffer<Req, F>
 where
-    T: Service<Request>,
-    T::Error: Into<crate::BoxError>,
+    F: 'static,
 {
     /// Creates a new [`Buffer`] wrapping `service`.
     ///
@@ -47,12 +46,12 @@ where
     /// [`Poll::Ready`]: std::task::Poll::Ready
     /// [`call`]: crate::Service::call
     /// [`poll_ready`]: crate::Service::poll_ready
-    pub fn new(service: T, bound: usize) -> Self
+    pub fn new<S>(service: S, bound: usize) -> Self
     where
-        T: Send + 'static,
-        T::Future: Send,
-        T::Error: Send + Sync,
-        Request: Send + 'static,
+        S: Service<Req, Future = F> + Send + 'static,
+        F: Send,
+        S::Error: Into<crate::BoxError> + Send + Sync,
+        Req: Send + 'static,
     {
         let (service, worker) = Self::pair(service, bound);
         tokio::spawn(worker);
@@ -64,12 +63,12 @@ where
     /// This is useful if you do not want to spawn directly onto the tokio runtime
     /// but instead want to use your own executor. This will return the [`Buffer`] and
     /// the background `Worker` that you can then spawn.
-    pub fn pair(service: T, bound: usize) -> (Buffer<T, Request>, Worker<T, Request>)
+    pub fn pair<S>(service: S, bound: usize) -> (Self, Worker<S, Req>)
     where
-        T: Send + 'static,
-        T::Error: Send + Sync,
-        Request: Send + 'static,
-        T::Future: Send + 'static,
+        S: Service<Req, Future = F> + Send + 'static,
+        F: Send,
+        S::Error: Into<crate::BoxError> + Send + Sync,
+        Req: Send + 'static,
     {
         let (tx, rx) = mpsc::channel(bound);
         let (handle, worker) = Worker::new(service, rx);
@@ -85,16 +84,15 @@ where
     }
 }
 
-impl<T, Request> Service<Request> for Buffer<T, Request>
+impl<Req, Rsp, F, E> Service<Req> for Buffer<Req, F>
 where
-    T: Service<Request>,
-    T::Error: Into<crate::BoxError>,
-    T::Future: Send + 'static,
-    Request: Send + 'static,
+    F: Future<Output = Result<Rsp, E>> + Send + 'static,
+    E: Into<crate::BoxError>,
+    Req: Send + 'static,
 {
-    type Response = T::Response;
+    type Response = Rsp;
     type Error = crate::BoxError;
-    type Future = ResponseFuture<T::Future>;
+    type Future = ResponseFuture<F>;
 
     fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
         // First, check if the worker is still alive.
@@ -109,7 +107,7 @@ where
             .map_err(|_| self.get_worker_error())
     }
 
-    fn call(&mut self, request: Request) -> Self::Future {
+    fn call(&mut self, request: Req) -> Self::Future {
         tracing::trace!("sending request to buffer worker");
 
         // get the current Span so that we can explicitly propagate it to the worker
@@ -132,11 +130,10 @@ where
     }
 }
 
-impl<T, Request> Clone for Buffer<T, Request>
+impl<Req, F> Clone for Buffer<Req, F>
 where
-    T: Service<Request>,
-    Request: Send + 'static,
-    T::Future: Send + 'static,
+    Req: Send + 'static,
+    F: Send + 'static,
 {
     fn clone(&self) -> Self {
         Self {


Test patch:
diff --git a/tower/tests/buffer/main.rs b/tower/tests/buffer/main.rs
index b65de6b..ee238f1 100644
--- a/tower/tests/buffer/main.rs
+++ b/tower/tests/buffer/main.rs
@@ -437,15 +437,15 @@ async fn doesnt_leak_permits() {
     assert_ready_ok!(ready3.poll());
 }
 
-type Mock = mock::Mock<&'static str, &'static str>;
 type Handle = mock::Handle<&'static str, &'static str>;
+type MockBuffer = Buffer<&'static str, mock::future::ResponseFuture<&'static str>>;
 
-fn new_service() -> (mock::Spawn<Buffer<Mock, &'static str>>, Handle) {
+fn new_service() -> (mock::Spawn<MockBuffer>, Handle) {
     // bound is >0 here because clears_canceled_requests needs multiple outstanding requests
     new_service_with_bound(10)
 }
 
-fn new_service_with_bound(bound: usize) -> (mock::Spawn<Buffer<Mock, &'static str>>, Handle) {
+fn new_service_with_bound(bound: usize) -> (mock::Spawn<MockBuffer>, Handle) {
     mock::spawn_with(|s| {
         let (svc, worker) = Buffer::pair(s, bound);