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

Server should be an executable Future, not an executor (shim) in its own right #422

Open
vorot93 opened this issue Apr 19, 2019 · 3 comments

Comments

@vorot93
Copy link

vorot93 commented Apr 19, 2019

jsonrpc's ServerBuilder accepts a custom executor or starts its own and then requires the user to start it.

On the other hand, both hyper and tarpc build a Server struct that is a plain Future that can be spawned on user's executor of choice. It is way more ergonomic and in line with async ecosystem's move towards separation of logic and its execution.

@tomusdrw
Copy link
Contributor

@vorot93 I agree 100%. Old API is more of a legacy from the times where tokio/futures and async in general was not really a thing yet, we should think on rewriting this if all transports support async.

@christobias
Copy link

christobias commented Jul 22, 2019

With a cursory look through the current code, IMHO since ServerHandler is a valid hyper Service, to support this without a breaking API change it would be a good idea to either:

  1. Allow taking the ServerHandler from an existing Server, or
  2. Split the creation of ServerHandler from Server::serve_http() and provide a way to create Server instances from existing ServerHandlers

The latter option would be a much better fit because users who require just the handler (because they have an existing tokio runtime or hyper instance) can utilize their existing code without extra overhead, while giving a simpler ready-made instance for easier setup

christobias added a commit to christobias/jsonrpc that referenced this issue Aug 2, 2019
Allows users to attach jsonrpc to an existing hyper server
@vorot93
Copy link
Author

vorot93 commented Jul 30, 2020

Actually, this may no longer be a valid issue, given that hyper spawns tasks itself now.

Where I'm contributing (e.g. rust-devp2p) I'm leaning towards such hybrid approach:

impl Server {
    fn new(executor) -> Self
}

impl Future for Server {
    ....
}

Server itself may spawn tasks e.g. to handle each request, while providing a future that resolves when the server is shut down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants