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

Creating Server with a Handle #1322

Closed
seanmonstar opened this issue Sep 16, 2017 · 3 comments
Closed

Creating Server with a Handle #1322

seanmonstar opened this issue Sep 16, 2017 · 3 comments
Labels
A-server Area: server. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful.
Milestone

Comments

@seanmonstar
Copy link
Member

I've put this off for a while claiming that it will be solved in tokio-proto, but I'm now leaning towards may as well fix it in hyper, if a generic way appears in the future, no harm done. So, on to the proposal:


I hinted at a possible solution earlier in another issue, and I think a solution is probably similar: Add a way to combine an Http, Service, TcpListener, and Handle into a impl Future.

Proposed API

  • Http
    • pub fn bind_handle<S>(&self, addr: &SocketAddr, new_service: S, handle: &Handle) -> Server
  • Server
    • pub fn shutdown_signal<F>(&mut self, signal: F) -> &mut Self
    • impl Future for Server

Usage

// Spawn 2 servers on the same Core
let mut core = Core::new()?;
let handle = core.handle();

let srv1 = Http::new()
    .bind_handle(addr1, new_service1, &handle);

let srv2 = Http::new()
    .bind_handle(addr2, new_service2, &handle);

handle.spawn(srv1);
handle.spawn(srv2);
core.run(futures::future::empty())?;
// Spawn a Server with a Client
let mut core = Core::new()?;
let handle = core.handle();

let client = Client::new(&handle);
let server = Http::new()
    .bind_handle(addr, new_service(client), &handle);
handle.spawn(server);

core.run(futures::future::empty())?;
// Spawn a Server and still get graceful shutdown
let mut core = Core::new()?;
let handle = core.handle();

// send on tx to shutdown server future
let (tx, rx) = oneshot::channel();

let client = Client::new(&handle);
let mut server = Http::new()
    .bind_handle(addr, new_service(client), &handle);
server.shutdown_signal(rx);

// can even just run server directly
core.run(server)?;

Implementation

This proposal is reusing the Server type, which already exists in a form that owns a Core. Internally, it can just change its field to an enum of 2 variants, 1 with a Core, and the other with a Handle. It should be able to use the majority of the same code in both cases.

It might be tricky that you can create a Server using Http::bind, which owns a Core, and then you can send that Server into another Core, calling handle.spawn(server). If you were to do that, the impl Future for Server could make the outer Core lock up, since an implementation might decide to call core.run() on its inner Core.

To prevent that mistake, the impl Future for Server should probably panic if the Server owns a Core, and not a Handle, with a nice message explaining don't do that.

/cc #1075 #1263

@seanmonstar seanmonstar added A-server Area: server. E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful. C-feature Category: feature. This is adding a new feature. and removed E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work. labels Sep 16, 2017
@algermissen
Copy link

Caveat: I am very(!) new to this, and merely trying to add value here, not complaining.

FWIW it feels that the current convenience impl and also the proposals hide too much of the internals (or do not allow for working with them through accessors). Eg one aspect is listening on a port with multiple threads (eg [1]). Another one is related to handling failures in the incoming conns loop (sorry, need to dig the reference up). So I think a more fine grained API will be beneficial.

[1] https://github.com/algermissen/web-rust/blob/master/src/bin/ts1.rs#L50

@seanmonstar
Copy link
Member Author

@algermissen Thanks for the comment!

Eg one aspect is listening on a port with multiple threads.

Sure. A recent PR, #1326, suggested allowing the Server to be able to use any Stream of IO objects, not just a TcpListener. I've wanted this too, but felt it was different enough to not be in this issue. With such a change, one could configure a TcpListener before giving to hyper, which would allow setting SO_REUSEPORT, and then you could have a server running on several threads with the same port.

Another one is related to handling failures in the incoming conns loop.

Again, by customizing a Stream of connections, one can completely decide what to do with errors coming in the stream. FWIW, errors from accept are usually errors from the pending connection, which means that connection should probably just be dropped, and one can accept again.

@kw217
Copy link
Contributor

kw217 commented Sep 25, 2017

This would definitely be an improvement - thanks.

In terms of documentation, please can you consider my concerns from #1207 (comment), specifically making it clear that Server is just a helper and you can build a more flexible server by using Http directly? That's still the case, even if this current change makes Server more powerful.

As a newbie, I clicked through to hyper::server which lists four structs, one of which is "Server: an instance of a server created through Http::bind". So I went to Http::bind and it seemed to do what I wanted. Immediately below is Http::bind_connection but it says "This is the low-level method ... This method is typically not invoked directly but is rather transitively used through bind."

I think those statements are a bit too strong.

  • Firstly, despite the name, Server is just another server - it's not special in any way. Http is the key struct for HTTP servers. Can the first sentence of the Server comment say "Convenience type used by Http:bind", so it's more obvious this isn't the primary type?
  • Secondly, can Http::bind say "if you need more flexibility, use bind_connection" and Http::bind_connection remove the comments about "low-level" and "not typically invoked directly"? Instead it could say "For simple cases consider using bind instead".

@seanmonstar seanmonstar added this to the 0.12 milestone Mar 17, 2018
seanmonstar added a commit that referenced this issue Apr 16, 2018
The `hyper::Server` is now a proper higher-level API for running HTTP
servers. There is a related `hyper::server::Builder` type, to construct
a `Server`. All other types (`Http`, `Serve`, etc) were moved into the
"lower-level" `hyper::server::conn` module.

The `Server` is a `Future` representing a listening HTTP server. Options
needed to build one are set on the `Builder`.

As `Server` is just a `Future`, it no longer owns a thread-blocking
executor, and can thus be run next to other servers, clients, or
what-have-you.

Closes #1322
Closes #1263

BREAKING CHANGE: The `Server` is no longer created from `Http::bind`,
  nor is it `run`. It is a `Future` that must be polled by an
  `Executor`.

  The `hyper::server::Http` type has move to
  `hyper::server::conn::Http`.
seanmonstar added a commit that referenced this issue Apr 16, 2018
The `hyper::Server` is now a proper higher-level API for running HTTP
servers. There is a related `hyper::server::Builder` type, to construct
a `Server`. All other types (`Http`, `Serve`, etc) were moved into the
"lower-level" `hyper::server::conn` module.

The `Server` is a `Future` representing a listening HTTP server. Options
needed to build one are set on the `Builder`.

As `Server` is just a `Future`, it no longer owns a thread-blocking
executor, and can thus be run next to other servers, clients, or
what-have-you.

Closes #1322
Closes #1263

BREAKING CHANGE: The `Server` is no longer created from `Http::bind`,
  nor is it `run`. It is a `Future` that must be polled by an
  `Executor`.

  The `hyper::server::Http` type has move to
  `hyper::server::conn::Http`.
seanmonstar added a commit that referenced this issue Apr 16, 2018
The `hyper::Server` is now a proper higher-level API for running HTTP
servers. There is a related `hyper::server::Builder` type, to construct
a `Server`. All other types (`Http`, `Serve`, etc) were moved into the
"lower-level" `hyper::server::conn` module.

The `Server` is a `Future` representing a listening HTTP server. Options
needed to build one are set on the `Builder`.

As `Server` is just a `Future`, it no longer owns a thread-blocking
executor, and can thus be run next to other servers, clients, or
what-have-you.

Closes #1322
Closes #1263

BREAKING CHANGE: The `Server` is no longer created from `Http::bind`,
  nor is it `run`. It is a `Future` that must be polled by an
  `Executor`.

  The `hyper::server::Http` type has move to
  `hyper::server::conn::Http`.
seanmonstar added a commit that referenced this issue Apr 16, 2018
The `hyper::Server` is now a proper higher-level API for running HTTP
servers. There is a related `hyper::server::Builder` type, to construct
a `Server`. All other types (`Http`, `Serve`, etc) were moved into the
"lower-level" `hyper::server::conn` module.

The `Server` is a `Future` representing a listening HTTP server. Options
needed to build one are set on the `Builder`.

As `Server` is just a `Future`, it no longer owns a thread-blocking
executor, and can thus be run next to other servers, clients, or
what-have-you.

Closes #1322
Closes #1263

BREAKING CHANGE: The `Server` is no longer created from `Http::bind`,
  nor is it `run`. It is a `Future` that must be polled by an
  `Executor`.

  The `hyper::server::Http` type has move to
  `hyper::server::conn::Http`.
seanmonstar added a commit that referenced this issue Apr 16, 2018
The `hyper::Server` is now a proper higher-level API for running HTTP
servers. There is a related `hyper::server::Builder` type, to construct
a `Server`. All other types (`Http`, `Serve`, etc) were moved into the
"lower-level" `hyper::server::conn` module.

The `Server` is a `Future` representing a listening HTTP server. Options
needed to build one are set on the `Builder`.

As `Server` is just a `Future`, it no longer owns a thread-blocking
executor, and can thus be run next to other servers, clients, or
what-have-you.

Closes #1322
Closes #1263

BREAKING CHANGE: The `Server` is no longer created from `Http::bind`,
  nor is it `run`. It is a `Future` that must be polled by an
  `Executor`.

  The `hyper::server::Http` type has move to
  `hyper::server::conn::Http`.
seanmonstar added a commit that referenced this issue Apr 16, 2018
The `hyper::Server` is now a proper higher-level API for running HTTP
servers. There is a related `hyper::server::Builder` type, to construct
a `Server`. All other types (`Http`, `Serve`, etc) were moved into the
"lower-level" `hyper::server::conn` module.

The `Server` is a `Future` representing a listening HTTP server. Options
needed to build one are set on the `Builder`.

As `Server` is just a `Future`, it no longer owns a thread-blocking
executor, and can thus be run next to other servers, clients, or
what-have-you.

Closes #1322
Closes #1263

BREAKING CHANGE: The `Server` is no longer created from `Http::bind`,
  nor is it `run`. It is a `Future` that must be polled by an
  `Executor`.

  The `hyper::server::Http` type has move to
  `hyper::server::conn::Http`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-server Area: server. C-feature Category: feature. This is adding a new feature. E-medium Effort: medium. Some knowledge of how hyper internal works would be useful.
Projects
None yet
Development

No branches or pull requests

3 participants