Consider changing middleware errors to Box<dyn Error>

[seanmonstar]

The Problem

Most of the various middlewares in tower define an enum Error<E> { Inner(E), .. } to return from Service. A problem starts to arise once you compose multiple middlewares together, but still want to be somewhat generic over handling the errors. Look at this gem:

error: Inner(Inner(Inner(B(Service(Inner(A(B(Error { kind: Connect, details: "interesting" }))))))))

In this case, we've wrapped a hyper::Client in a bunch of middleware. In order to inspect and react to the error (in my case, decide if the error qualifies to retry the request), I could:

• Set my expect error type exactly, and thus use match e { UnNestAllTheLayers => () }. This is very fragile, since adding new middleware, or changing the order, will cause compilation to fail. It's also hugely tedious being sure we've match every correct combination.
• Make some trait CanRetry and implement it for every single intermediary type. Adding a new middleware means we need to implement CanRetry for the potentially new nested error enum.

Proposal

A different solution might be better: remove the enum Error<E>, and instead having all middleware just return Box<dyn std::error::Error + Send + Sync>.

The code needed to inspect errors if they were dynamic trait objects changes to something like this:

if let Some(e) = error.downcast_ref::<hyper::Error>() {
    if e.is_connect() { // or whatever
        return CanRetry::Yes;
    }
}

Example

Consider tower-in-flight-limit:

impl<S, Req> Service<Req> for InFlightLimit<S>
where
    S: Service<Req>,
    S::Error: Into<Box<dyn StdError + Send + Sync>>,
{
    type Response = S::Response;
    type Error = Box<dyn StdError + Send + Sync>;

    // ...
}

impl<T> Future for ResponseFuture<T>
where
    T: Future,
    T::Error: Into<Box<dyn StdError + Send + Sync>>,
{
    type Item = T::Item;
    type Error = Box<dyn StdError + Send + Sync>;

    fn poll(&mut self) -> Poll<Self::Item, Self::Error> {
        match self.inner {
            Some(ref mut f) => {
                match f.poll() {
                    // ...
                    Err(e) => {
                        self.shared.release();
                        Err(e.into())
                    }
                }
            }
            None => Err(NoCapacity.into()),
        }
    }
}

This change means that if an error occurs, it will be a Box<dyn StdError + Send + Sync>, but it won't be nested (unless nesting with Error::cause() (now in 1.30, Error::source()!)), and so it should only be either S::Error or NoCapacity.

By requiring the bounds of S::Error: Into<Box<dyn StdError + Send + Sync>>, this allows only boxing the error once. If multiple middleware are wrapped up, and where they would previously return Error::Inner(e), they'll just return the Box<dyn StdError> without wrapping.

A couple downsides exist with this change:

• By moving to trait objects, we can no longer rely on Send and Sync being propagated through the generics, so we would need to require S;:Error: Send + Sync. In practice, it seems reasonable that errors would be Send + Sync anyways.
• This means the error case now causes an allocation, whereas with the enum, it didn't. The error cause should be infrequent, so the allocations shouldn't happen too often. However, this actually can mean an performance improvement, since Result<T, Box<Whatever>> can sometimes remove the need for the tag in Result, if T is zero-sized. Additionally, nested enums grow in memory, so having a single Box can means the Result is smaller regardless.
• If you aren't nesting middleware, then the sum type enum Error can be nice to ensure you handle every error case, and this proposal would force you to do downcast even in those simple cases.

[seanmonstar]

cc @carllerche @hawkw @olix0r @jonhoo

[davidbarsky]

I'd really like to use Tower in aws-lambda-rust-runtime, and this would make our (expected) nested middleware stack much nicer.

[carllerche]

@davidbarsky could you provide more specifics about how it would make it nicer?

[hawkw]

I'd prefer to avoid returning Box<dyn Error> if at all possible. The main reason is because I've seen cases (such as in Linkerd) where traits other than Error are implemented for error types, and returning boxed trait objects would erase those additional traits. There are also the additional downsides that @seanmonstar mentioned above to consider, but the erasure of non-Error traits is my primary concern.

This doesn't resolve all the same issues as returning boxed trait objects, but one potential solution to the problem of unreadable debug output for errors is to just use the fmt::Display implementations instead. I believe in most cases, Tower's error types implement fmt::Display by proxying to the inner error, when they wrap another error. Using fmt::Display to log these errors would result in only the message of the original error type being logged, rather than a complex nested type.

[seanmonstar]

one potential solution to the problem of unreadable debug output for errors is to just use the fmt::Display implementations instead.

I didn't mean to say this was to fix debug output, I simply was showing what the type structurally looked like inside that function, and what match I would need to write to check for only certain variants.

For example, I'd need to write:

fn can_retry(err: &router::Error<buffer::Error<balance::Error<Either<Etc, MoreEtc>>>>) {
    match err {
        route::Error::Inner(buffer::Error::Inner(balance::Error::Inner(Either::A(reconnect::Error::Inner(Etc))))) => {}
    }
}

[davidbarsky]

@carllerche

Sure. I expect that we'd have a nested Tower middleware stack (starting with tower-retry, tokio-trace-tower, tower-balance) before any request even reaches user-level code. As Sean noted in his proposal, this would reduce the fragility of the middleware stack. Additionally, I suspect that a large chunk of errors deep within a middleware stack can't reasonably be handled by other middleware.

@seanmonstar

Perhaps I'm missing something—would this prevent returning something like the Never type? Additionally—and this is probably out of scope for this change—but are there plans to change tower's associated types to a futures 0.3-style that return a single Output type?

[seanmonstar]

It wouldn't prevent returning Never/!, since they can implement std::error::Error, but it would reduce the optimizations that could be performed because of it being uninhabited would be lost to the type system at higher levels.

FWIW, I'm not convinced this proposal is the best, just thought I'd put together my thoughts on my current experience trying to determine what protocol and Middleware errors could be safely retried.

[carllerche]

@seanmonstar I assume that this would just be a convention used by middleware, not a change to the Service trait?

[seanmonstar]

Correct, I mean only as a pattern of middleware.

[carllerche]

@seanmonstar what’s the conclusion?

[davidbarsky]

Given the discussion in #141 and my limited experience with Tower composing Tower's middleware I think I'm in favor of this change for a few reasons:

• Tower already encourages a large degree of nested types and reducing the amount of nested types an end-customer needs to worry about is a win IMO.
• I've already mentioned this in a prior comment, but given my suspicion that a large chunk of errors deep within a middleware stack can't be handled by other middleware layers, doing some type erasure via the cast to a boxed object is appealing. Plucking out specific errors using error.downcast_ref appears to be a pattern people are gravitating with Failure and Error::source().
• A pain-point I encountered with Tower is composing different error types between middleware layers. It might be me misusing Tower, but I suspect that other users might run into the same issue.

As Sean said,

Set my expect error type exactly, and thus use match e { UnNestAllTheLayers => () }. This is very fragile, since adding new middleware, or changing the order, will cause compilation to fail. It's also hugely tedious being sure we've match every correct combination.

I strongly concur on this point.