Should `TimeoutLayer` middleware allow returning other status codes?

[mlodato517]

Feature Request

Motivation

I'd like to be able to return a 504 if the timeout given to the TimeoutLayer is exceeded. This is because a 408, which is currently returned, is meant to communicate:

that the server did not receive a complete request message within the time that it was prepared to wait.

I don't know if in general we can tell if the issue is the client sending the request or the server processing the request but in the case of the latter (especially because this service is proxying to another) I'd like to return a 504 Gateway Timeout instead (which is still not a guarantee - the issue could have been the upstream or any post-processing of the upstream's response but that's rare in this case). Servers like axum and actix-web provide other mechanisms for timeouts when receiving things like request headers^1 so maybe something about receiving the request should be handled at the server level or by something like BodyTimeout?

Proposal

Changing the response status from 408 all the time to 504 all the time would equally be incorrect sometimes and would be a breaking change. Luckily we already have a new() constructor for TimeoutLayer so I imagine that could stay the same and default to 408 and then we could have a new_with_response constructor that accepts an http::Response<B> or similar that can be returned instead.

Alternatives

The most obvious alternative I can see follows from this comment which is to use tower instead of tower-http and explicitly catch the error and perform custom logic on it. This isn't too much code but I wonder if the alternate constructor is valuable because I'm not sure how frequent 408 will be the desired response code (though HTTP semantics are fuzzy enough that maybe it's not worth it!).

[davidpdrsn]

Servers like axum and actix-web provide other mechanisms for timeouts when receiving things like request headers

From a tower middleware it's not possible to distinguish between reading the request head timing out or the server handling the request. That requires a specific integration into hyper.

I think if you want a different status you can write your own middleware for it. It's not complicated. If you're using axum it's even easier with axum::middleware::from_fn.

[mlodato517]

From a tower middleware it's not possible to distinguish between reading the request head timing out or the server handling the request. That requires a specific integration into hyper.

Totally agreed. Which makes me feel even more like this middleware shouldn't be assuming the error is a client-side error :thinking:

[davidpdrsn]

But not all servers are proxies so 504 doesn't seem right to me.

The HyperText Transfer Protocol (HTTP) 504 Gateway Timeout server error response code indicates that the server, while acting as a gateway or proxy, did not get a response in time from the upstream server that it needed in order to complete the request.

[mlodato517]

But not all servers are proxies so 504 doesn't seem right to me.

Ah totally agreed there - that's why I was hoping to make it configurable. Basically "this middleware is generic and can be used anywhere and so can't pick a status code and be correct all the time". So we can leave the default as 408 so it's not a breaking change but if we give consumers another constructor/method to provide a custom Response[^1] then it can be used anywhere[^2].

[^1]: Or maybe just a status code for now but a Response object would be more flexible at the end of the day [^2]: I'm sure there are places it can't be used but this at least expands where it can be used! :crossed_fingers:

[seanmonstar]

Often I've seen servers return 503 when their own internal timeout triggered (not as a gateway). :)

[davidpdrsn]

Sure we can add a method to change the status code.

I don't think we should allow customizing the response body. If people need that I'd recommend writing your own middleware.

[mlodato517]

Yeah I can see adding the Response would be annoying because of the generic body. It's a micro-bummer because, in the case of a 503, for instance, the server may want to add a Retry-After header or something but I think a status code is a reasonable compromise for now!

[seanmonstar]

A lot of inspiration for tower came from Finagle, so if there's some prior art for how they handle HTTP timeouts, we could, uh, be inspired again :)

[Kinrany]

To provide one example, Firefox appears to rely on 408 to mean "client failed to upload the request in time" to retry requests automatically. This makes sense according to the spec as far as I can tell.

This makes returning 408 the entirely wrong thing to do for timeouts caused by server-side processing of the request taking too long: the same slow operation will likely be executed and aborted every time in exactly the same way.

[mohe2015]

Firefox will retry the request 10 times in my testing when sending 408 so I think we should either change the default or at least make it configurable. I personally would really change the default as that looks like a big footgun.