Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a ReadyService trait #25

Merged
merged 2 commits into from Nov 21, 2017
Merged

Add a ReadyService trait #25

merged 2 commits into from Nov 21, 2017

Conversation

carllerche
Copy link
Member

This PR adds a ReadyService trait. A ReadyService is similar to Service except that it is always "ready" to accept a request. A ReadyService implementation can immediately error the response if it is "at capacity", but it has no strategy by which to notify the caller that it becomes ready again.

It is expected that types will usually implement either Service OR ReadyService (though, there may be cases in which a type can implement both). It is also expected that server implementations will only accept Service values.

Adapting

ReadyService -> Service

In order to adapt a ReadyService to a Service, I can think of a couple strategies off the top of my head.

Don't do anything special.

In this case, there will be some type AdaptReadyService (naming TBD) that takes a ReadyService and implements Service such that poll_ready always returns Ready. This requires the user to make an explicit step to say that the server will not have any mechanism to handle back pressure.

Add a limiter

In this case, a ReadyService value is passed to Limit::new(my_ready_service, n). This type ensures that there are only n in-flight requests. This limiter can be scoped to the connection (n in-flight requests per connection) or per "app" so that the number of in-flight requests are limited across many connections.

Service -> ReadyService

There are reasons to want to adapt the other direction as well. For example a router (a single service that routes a request to one of many possible inner services) cannot easily provide a back pressure strategy. In this case, a router would want to be initialized with a set of ReadyService values.

To adapt a ReadyService to Service, the primary strategy would be to add a buffering layer. This buffer would ideally have an upper limit. When the upper limit is reached, the buffer starts erroring requests immediately.

Higher level frameworks

Obviously, introducing a new trait adds non trivial complexity. That said, I would expect that ReadyService would provide a better suited trait for reducing initial friction. I would not necessarily expect frameworks to ask their users to implement ReadyService, but frameworks could use this trait internally and build up a tower stack using Router, Limit, Buffer, etc... and eventually provide a Service representing the entire stack and pass that to a server.

Alternatives

There are two alternatives to introducing ReadyService.

Keep things as is.

In this case, a "ready service" would be a type that implements Service such that poll_ready always returns Ready. The service would need to document that poll_ready does not need to be called before call (as there are some services that require poll_ready to return Ready before calling call).

The router type would accept a set of Service values, documenting that these must be "ready service" values and as such poll_ready will not be called. This means, if a service that requires poll_ready to be called is passed, it would just hang (for example, Reconnect).

Remove poll_ready from Service.

If ReadyService is needed, is poll_ready actually helpful? I would say yes as most middleware implementations so far heavily rely on poll_ready to do the "right thing". The only middleware that I have written that requires a "ready service" is the router.

@carllerche
Copy link
Member Author

Incidentally, the actual PR diff is more an illustration of the trait than anything. If merged, docs, etc... would need to be added and we would probably want to add combinators like ReadyService::limit that makes adapting to Service easier.

@carllerche
Copy link
Member Author

carllerche commented Nov 2, 2017

This also relates to #21 and #11.

@olix0r
Copy link
Collaborator

olix0r commented Nov 2, 2017

It's interesting that ReadyService is a pure subset of Service. Though, I suppose we don't want to express trait Service: ReadyService, as this would allow instances to be used as ReadyServices when they aren't necessarily ready...

I think this change seems fine to me, once the mentioned helpers/combinators are introduced.

@seanmonstar
Copy link
Collaborator

Should this be a separate trait? It really sounds like a specialized Service, in a similar vein to a ConstService or something.

struct ReadyService<F>(F);

impl<F, Req, Res, Err, Fut> Service for ReadyService<F>
where
    F: FnMut(Req) -> Fut,
    Fut: Future<Item=Res, Error=Err>,
{
    type Request = Req;
    type Response = Res;
    type Error = Err;
    type Future = Fut;

    fn poll_ready(&mut self) -> Poll<(), Self::Error> {
        // always ready
        Ok(Async::Ready(()))
    }

    fn call(&mut self, req: S::Request) -> S::Future {
        (self.0)(req)
    }
}

@carllerche
Copy link
Member Author

@seanmonstar The reason I didn't make it a specialization of Service is that we can use typing to enforce a back pressure mechanism is used. This would require a ReadyService to be wrapped by a Limit before being passed to a server.

It would also force an explicit step before having a server run an unbounded service:

server.run(Unbounded(MyReadyService))

@carllerche
Copy link
Member Author

Unless there are objections, I am going to merge this and create an issue tracking that the utility of ReadyService should be evaluated before releasing 0.1.

@withoutboats
Copy link
Contributor

Looks good to me. :)

@carllerche
Copy link
Member Author

I added more documentation. Additional helpers, such as limit, will be deferred to another PR.

@carllerche carllerche merged commit 72e4b98 into master Nov 21, 2017
@carllerche carllerche deleted the ready-service branch November 21, 2017 20:56
@hawkw hawkw mentioned this pull request Mar 31, 2020
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants