tower

tower copied to clipboard

This is a possible solution to #111. The idea is to share the service itself between Buffer and Worker using an Arc<Mutex<Option<T>>>. Buffer::try_into_inner then takes the inner value if its there.

After extracting the inner service all requests will fail with ServiceExtractedFromWorker.

I would like some input on this 😊

• Is this even a feature we want to provide? #111 is quite old has hasn't seen much activity. Buffer is currently an odd one in that it doesn't currently have an into_inner. Most other middlewares have that.
• Would this implementation work? I imagine the Arc<Mutex<_>> would add a small amount of overhead. I haven't benchmarked the before/after.

cc @jonhoo

---

Fixes #111

Thanks for picking this up! I still think this would be a welcome addition.

The overhead of an uncontended Mutex is hard to measure, and both hardware and software dependent. In the best case, it reduces to just a compare-exchange, which in turn at best is the same cost as a load+store. But since we're talking about every interaction with Service, I'd like to see us avoid it if we can. One way to do this would be to share an Arc<Mutex<Option<S>>> instead, and have the spawned task only put the Service in there if it errors. This will require some more code, though I think maybe you could pull it off cleanly with a sneaky macro. I'd be curious to hear others' thoughts here too though — this may be over-optimizing.

There's also the point that this increases the size of the handle to now also hold the Arc, but that I'm less worried about.

Hmm, I wonder if this can be implemented in a more modular way, outside of buffer, so that users who don't need to use this don't have to pay for it?

Maybe something like

pub struct StealableService<T> {
    service: Arc<Mutex<Option<T>>>,
}

pub struct Steal<T>(Arc<Mutex<Option<T>>>);

impl<T> StealableService<T> {
    pub fn new(service: T) -> (Self, Steal<T>) {
        let service = Arc::new(Mutex::new(Some(service)));
        let steal = Steal(service.clone());
        (Self { service }, steal)
    }
}

impl<T, R> Service<R> for StealableService<T>
where
    T: Service<R>,
    T::Error: Into<crate::BoxError>,
{
     // ...
}

impl<T> Steal<T> {
    pub fn steal_service(&self) -> Option<T> {
        self.0.lock().unwrap().take()
    }
}

Then, you could just put a StealableService inside the buffer, and use the handle type to steal the service. The overhead of the Arc<Mutex<...>> is now only added when you actually need the ability to steal the inner service, and not in the common case where you don't.

Would something like this work? The downside is that there is now a separate type for stealing the service, rather than being able to steal it from the Buffer type directly. But, you could use this to write a wrapper around buffer, like this:

#[derive(Clone)]
pub struct StealableBuffer<T> {
    buffer: Buffer<StealableService<T>>,
    steal: Steal<T>,
}

impl<T> StealableBuffer<T> { 
    pub fn new(service: T) -> (Self, buffer::Worker<StealableService<T>>) {
        let (service, steal) = StealableService::new(service);
        let (buffer, worker) = Buffer::new(service);
        let service = Self { buffer, steal };
        (service, worker)
    }

    pub fn try_into_inner(&self) -> Option<T> {
        self.steal.steal_service()
    }
}

impl<T, R> Service<T> for StealableService<T>
where
    T: Service<R>,
{
    type Response = T::Response;
    type Error = crate::BoxError;
    type Future = buffer::ResponseFuture<T::Future>;

    fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
       self.buffer.poll_ready(cx)
    }

    fn call(&mut self, req: R) -> Self::Future {
        self.buffer.call(req)
    }
}

Uh I think that's an interesting idea. Might just work. @jonhoo What do you think?

i would be fine with a PR to add something like that in tower, although i think it should be fairly straightforward for users to implement themselves.

out of curiosity, what's the use-case you have in mind for this? if it's something a lot of people might need, having some interface in tower for it could be a good idea...

The downside of that approach is that you now need to take the Mutex on every poll_ready/call, just as in the current version of this PR. What I was proposing instead was to keep the service inside the Buffer directly (not behind a Mutex) during normal operation, and then only take the Mutex to stick the service in there when an error occurs.

I do like your proposal though @hawkw — I was hoping for a second we could get the best of both worlds with something like an AtomicBool + Mutex + Condvar combo, where to steal you first set the bool to true, then you wait on a Condvar associated with the Mutex for the thing that owns the service to give it away. And inside the service, every call to poll_ready/call checks the bool first. Plus maybe a method for voluntarily giving out the service (which Buffer would call on error). But unfortunately the wrapping service needs to know about this functionality, and the inner service may not be returned for quite some time because the owning service is asleep... Overall, it doesn't sound very attractive.

My instinct here is to not generalize unless we need it here, and special-case this for Buffer. The reason I'm thinking that is that most services can just have an into_inner to unwrap the service, and Buffer is in the special case where it cannot do that.