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

expose tokio::net::tcp_listener? #12

Closed
yoshuawuyts opened this issue Jul 13, 2018 · 4 comments
Closed

expose tokio::net::tcp_listener? #12

yoshuawuyts opened this issue Jul 13, 2018 · 4 comments
Labels
enhancement New feature or request

Comments

@yoshuawuyts
Copy link
Collaborator

yoshuawuyts commented Jul 13, 2018

In #11 we're adding an example of how to use this crate with the Hyper HTTP server. To get it to work, some glue is required:

  let args = Cli::from_args();
  let listener = args.port.bind()?;
  let handle = tokio::reactor::Handle::current();
  let listener = tokio::net::TcpListener::from_std(listener, &handle)?;

This seems like boilerplate that would probably be copy-pasted from between projects, which makes it seem like a prime candidate to create a method for.

I was thinking something like this:

  let args = Cli::from_args();
  let listener = args.port.bind_tokio()?;

The name here could probably use some work (cc/ @killercup, suggestions?) but I reckon the result would be really useful.

Setting up a complete HTTP server would then only require the following:

#[derive(Debug, StructOpt)]
struct Cli {
  #[structopt(flatten)]
  port: Port,
}

fn main() -> Result<(), Box<std::error::Error>> {
  let args = Cli::from_args();
  let listener = args.port.bind_tokio()?;

  let server = Server::builder(listener.incoming())
    .serve(|| service_fn_ok(|_| Response::new(Body::from("Hello World"))))
    .map_err(|e| eprintln!("server error: {}", e));

  tokio::run(server);
  Ok(())
}

Which I think is starting to become short enough that people can memorize it!

Drawbacks

This would add tokio as a dependency which adds a bit of overhead during compilation. But given that all major frameworks depend on it, it's probably safe to say it won't introduce any new dependencies for production systems.

@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Jul 13, 2018
@killercup
Copy link

killercup commented Jul 14, 2018

Hm. Not sure I agree with this. I can see how this might come in handy, but I have two questions:

I think my main issue with this is that it is very specific to Hyper. If it were easier in hyper to use an existing socket, we wouldn't need this. In actix-web, e.g., you can pretty much to server.listen(args.port.bind()?) IIRC

@yoshuawuyts
Copy link
Collaborator Author

If it where easier in hyper to use an existing socket, we wouldn't need this.

Yeah I actually agree with this. Good point.

@yoshuawuyts
Copy link
Collaborator Author

yoshuawuyts commented Jul 17, 2018

Hmmm, on the other hand it would become harder to configure socket things like keepalive durations and the like. hyperium/hyper#1602 (comment)

edit: actually it looks like most options can be set on core's TcpStream struct. But tokio exposes more method, such as .set_recv_buffer_size(). This seems it should be mostly okay, and is the same recommendation actix-web has.

References

@yoshuawuyts
Copy link
Collaborator Author

Server::from_tcp() just landed in Hyper! hyperium/hyper#1624

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants