HTTP routers should use bounded buffering.

[olix0r]

Currently, the Inbound and Outbound HTTP routers use unbounded buffers:

impl<B> Recognize for Inbound<B> {
    // ...
    type Service = Buffer<bind::Service<B>>;

impl<B> Recognize for Outbound<B> {
    // ...
    type Service = Buffer<Balance<
        load::WithPendingRequests<Discovery<B>>,
        choose::PowerOfTwoChoices<rand::ThreadRng>,
    >>;

Each produces services wrapped in tower_buffer::Buffer

//! Buffer requests when the inner service is out of capacity.
//!
//! Buffering works by spawning a new task that is dedicated to pulling requests
//! out of the buffer and dispatching them to the inner service. By adding a
//! buffer and a dedicated task, the `Buffer` layer in front of the service can
//! be `Clone` even if the inner service is not.
//!
//! Currently, `Buffer` uses an unbounded buffering strategy, which is not a
//! good thing to put in production situations. However, it illustrates the idea
//! and capabilities around adding buffering to an arbitrary `Service`.

At a glance, it's not immediately obvious to me why Buffer needs to use mpsc::unbounded -- if we used mpsc::channel, I'd think Buffer::poll_ready could call mpsc::Sender::poll_ready. Is it more complicated than that? @carllerche @seanmonstar

[olix0r]

It's not actually just as simple as switching to a channel, since there's a race between poll_ready and call. We'll need to adopt an mpsc variant that reserves a slot in the channel in a way that makes this race impossible.

[seanmonstar]

I think the reason it is unbounded is because the router doesn't currently know what to do if the underlying service isn't ready. They're essentially ReadyServices, right?

So, if we put a limit on the buffer, then we need figure out what to do when that limit is hit. Respond with a 503? GOAWAY with ENHANCE_YOUR_CALM?

[carllerche]

The main issue is the main point of Buffer is to allow an arbitrary Service value to be Clone. If you have a non-clone service, wrapping it with Buffer adds a queue and spawns a task to manage that queue. The Buffer service is then clone.

Now, services that are Clone could potentially be cloned for each request so that the service handle can be moved into the response future. This is to support "and_then" kind of cases. With the current mpsc channel, cloning the Sender for each request is essentially an unbounded channel.

[briansmith]

If I understand @carllerche, it seems like we don't need to use Buffer at all since we control our whole stack of services and we can (AFAICT) make them all Clone. Is that what we should do?

I'm nominating this for 0.3 since it's a quality issue that affects one the central selling points of Conduit (reliability and resource consumption).

[carllerche]

@briansmith I don't recall all the details of the existing stack, but I believe that some flavors of middleware require the concept of Buffer in order to be Clone (in a futures world that is).

If all Service impls used in conduit are already Clone, then Buffer should probably be removed either way.

[olix0r]

@carllerche the issue is that things like Reconnect are not clone, and it would probably be complicated to make them Clone

[carllerche]

Ah yes... making Reconnect clone is wrapping it with a buffer layer.

[carllerche]

So, I believe that the strategy is to limit the max number of in-flight requests. As far as I can tell, there are two options:

• Limit on a per endpoint basis. Once the limit is reached, requests for that endpoint are dropped.
• Limit on a router basis. Once this limit is reached, accepting requests is paused until there is capacity.

What should the strategy be?

[olix0r]

@carllerche we should ideally be able to enforce this limit per endpoint so that slow endpoints can't DoS the whole proxy

[carllerche]

@olix0r Then, the strategy would be to "load shed" (drop) requests to slow endpoints. If that is OK, then the implementation is relatively straightforward.

[olix0r]

In progress: tower-rs/tower#49

[carllerche]

How do we want to set the max in-flight requests per endpoint?

[olix0r]

probably as part of the environment-defined configuration. I would also accept a const in the short-term...

[carllerche]

What would be a good default value?

[olix0r]

@carllerche at a guess, 10_000?