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

Provide a lower-level connection interface #1070

Closed
jebrosen opened this issue Aug 2, 2019 · 16 comments
Closed

Provide a lower-level connection interface #1070

jebrosen opened this issue Aug 2, 2019 · 16 comments
Assignees
Labels
enhancement A minor feature request
Milestone

Comments

@jebrosen
Copy link
Collaborator

jebrosen commented Aug 2, 2019

Currently Rocket's public API only provides a way to listen on TCP sockets (and Unix sockets have also been proposed) with optional TLS. Rather than adding support for every possible I/O layer in Rocket itself, it might be nice to provide an API for custom I/O types. Such an API now exists in http/src/listener.rs, but it is private. This issue is about making this or a similar API public, or integrating with another existing crate that provides such an API.


The original issue described here was implemented long ago without fanfare. Here's the original text:

hyper's AddrIncoming is not sufficient for our needs because it does not support TLS. We should implement an alternative to AddrIncoming that abstracts over plain vs TLS and (in the future) over TCP vs Unix sockets. This type should support most or all of the connection-level options hyper does, such as timeouts. It also needs to have a remote_addr method.

See also #594, which implements some changes to the configuration system to handle non-TCP addresses.

@jebrosen jebrosen self-assigned this Aug 29, 2019
@jebrosen
Copy link
Collaborator Author

jebrosen commented Sep 6, 2019

It would also be nice to support custom listeners. This would allow (among other things) custom TLS implementations using something other than rustls, unix sockets, or FastCGI to be implemented outside of Rocket.

Even if it's not exposed (yet), I think it's worth implementing one of these APIs internally, because it should make it somewhat easier to abstract over TLS and unix sockets which we definitely do want to support.

I have come up with two main ideas for a design of this.

"Rocket-driven":

rocket::ignite().listen_on::<ListenerType>(executor)

trait Listener {
    type Connection: Connection;
    
    async fn bind(address, keepalive, read_timeout, options...) -> Result<Self, io::Error>;
    fn local_addr(&self) -> Option<SocketAddr>;
    async fn accept(&mut self) -> Result<Self::Connection, io::Error>;
}

In this model, Rocket will call bind with the configured address and port. Its main weakness that I can see is the assumption there is an "address" to bind to, which would not be the case for unix sockets (a file path) or for something like CGI (stdin). #594 makes Address an enum of either Tcp or Unix, which is another possibility to consider here.

Note that listen_on as I wrote it takes an executor, similar to what was implemented in #1097.

User-driven:

rocket::ignite().listen_on(listener, executor)

trait Listener {
    type Connection: Connection;
    
    fn local_addr(&self) -> Option<SocketAddr>;
    fn set_keepalive(&mut self, keepalive);
    async fn accept(&mut self) -> Self::Connection;
}

In this model, it's assumed that the user will construct an instance of a listener ahead of time. Some parameters such as keepalive can be passed in by using setters, but configuration in Rocket.toml like address and port would have to be retrieved manually or ignored.

Connection

Either way, Listener::accept() returns a future resolving to its associated Connection type:

trait Connection: AysncRead + AsyncWrite {
    fn remote_addr(&self) -> SocketAddr;
    
    // This config could apply to the Listener trait
    // in which case it would be set in accept() instead.
    fn set_read_timeout(&mut self, timeout);
    
    // Exists mainly for closing the read half early
    fn close(&mut self, which);
}

@SergioBenitez, are there any obvious flaws in this kind of extensibility or issues that should be resolved now?

@jebrosen
Copy link
Collaborator Author

This is implemented in 8e103cb, but we will probably want to change some of the design before making any of the new types public.

@jebrosen
Copy link
Collaborator Author

@carllerche I believe Listener and Connection are the traits you mentioned the possibility of implementing upstream somewhere: 8e103cb#diff-fb2164547e31fbbe23ddbe7926331efaR18

I have two concerns about upstreaming them. First, where would the traits go? I have this (maybe unfounded) idea that there will be users of Rocket who don't want to or can't use tokio, so forcing a dependency on tokio_net for those traits might be annoying. The second concern I have is that we haven't yet explored things like keep_alive or connection idle timeout enough in async rocket to know which traits we might want to add which methods to. It would be unfortunate if a trait was created somewhere upstream but we needed to extend it, to some degree defeating the purpose of using that upstream trait.

@carllerche
Copy link

re: location, the idea is that HTTP related abstractions live in tower-http. This would be completely decoupled from hyper, tokio, and rocket. The intent of tower is to provide an interface between server / client implementations and the application (or app framework, like rocket).

@carllerche
Copy link

The second concern I have is that we haven't yet explored things like keep_alive or connection idle timeout enough in async rocket to know which traits we might want to add which methods to

tower-http is very much in experimentation phase too. We can experiment together and figure out where things fall (hyper, rocket, or tower). Maybe start by listing the "user stories" around keep alive / idle timeout.

@jebrosen
Copy link
Collaborator Author

There are a lot of layers to this problem surrounded by other irrelevant code, so I've written out a list of the steps performed that are relevant to connections.

For reference, the Listener and Connection types as they exist right now:

/// A 'Listener' yields incoming connections
pub trait Listener {
    type Connection: Connection;

    /// Return the actual address this listener bound to.
    fn local_addr(&self) -> Option<SocketAddr>;

    /// Try to accept an incoming Connection if ready
    fn poll_accept(&mut self, cx: &mut Context<'_>) -> Poll<Result<Self::Connection, io::Error>>;
}

/// A 'Connection' represents an open connection to a client
pub trait Connection: AsyncRead + AsyncWrite {
    fn remote_addr(&self) -> Option<SocketAddr>;
}

The current implementation:

  1. Rocket calls tokio_net::tcp::TcpListener::bind() or rocket_http::tls::TlsListener::bind() to create a Listener
    • Customization point: We (I?) want to allow crates to define their own Listener types (e.g. using a different TLS library, or Unix sockets)
  2. Rocket calls listener.local_addr() in order to print it out later
  3. Rocket wraps the listener into a Stream of incoming Connections with rocket_http::listener::Incoming::from_listener(listener)
  4. hyper::server::Builder::new(incoming) - Give hyper the incoming stream
  5. builder.serve(service) - Run the hyper server
  6. hyper calls incoming.poll_next()
    * Our Incoming type performs a delay-on-error like hyper's AddrIncoming does, but over any kind of connection
    1. Delay::poll() - Check if we have waited long enough since the last connection error
    2. Check if a connection is available
    1. Poll listener.accept()
    2. If there is a connection error, set a delay and return Pending (just like hyper's AddrIncoming)
    3. (TODO) Set socket idle timeout, keep-alive, etc.
      • This is the most uncertain part of the design. Should these be configured by rocket or by the application, and to what interface do they belong?
    4. Return the connection to hyper
  7. Hyper calls makeservice, service, whatever else, routes are handled, etc.
    1. As part of this, Rocket calls Connection::remote_addr()

Possible changes

  • local_addr() might have to return something other than SocketAddr for e.g. Unix sockets. However, remote_addr should be a real remote address if possible.

  • Some traits could be defined upstream:

    • Listener trait: has local_addr(&self) and poll_accept(&mut self, cx)
    • Connection trait: AsyncRead + AsyncWrite and remote_addr(&self)
  • Because of "Set socket idle timeout, keep-alive, etc.", Listener or Connection might have Rocket-specific requirements and thus need to be defined or redefined by Rocket.

    • Or, we could set those automatically in the implementations provided by Rocket, and require applications to implement them themselves when customizing.

Why do I keep bringing up keep-alive and idle timeouts?

Because they were there in Rocket 0.4. I'm only partly joking.

Rocket 0.4 needed a fairly short read timeout to prevent N_THREADS nonresponsive clients from blocking the server entirely. We don't need that in async Rocket because everything is asynchronous, but the other extreme of no timeout looks like a great way to run out of memory quickly so we should almost certainly have some configurable limit here.

Rocket 0.4 also provided a mechanism to set the keep-alive timeout (https://docs.rs/hyper/0.10.16/hyper/server/struct.Server.html#method.keep_alive). hyper's API has changed a bit and different knobs are exposed, and I haven't yet looked at the semantics of the various options and what would best approximate the previous behavior, if it's still desirable.

I actually hope that neither of these actually need to be part of the Listener or Connection APIs, but I don't know that for sure yet and I would not be surprised if we end up needing to control other aspects of the connection in some way or another.

To upstream or not to upstream

I think the main question left is where to set idle timeouts, keep-alive, or other connection settings I haven't thought of. I see two main options:

  • Rocket uses Listener and Connection from some upstream crate. I like this because it opens up the possibility to implement widely usable combinators. The main downside is that some connection configuration might need to be done manually by crates that use a listener type not provided by Rocket.
  • Rocket keeps defining Listener and Connection itself. This gives Rocket the most freedom to add configuration knobs like keep-alive and connection timeouts (even backward-compatibly with default implementations) that may be appropriate for web servers but not other kinds of network servers. The downside is the potential for duplication of work and adapter types.

@jcdickinson
Copy link

FYI: I had a stab at "async-std-runtime" this weekend and found a pretty important detail: poll_accept isn't available in async_std (or smol/async_io, its upstream dependency). This currently seems to be unique to tokio. I'll figure out if it can be added to each.

@jebrosen jebrosen changed the title (async) Implement a replacement for hyper's AddrIncoming Provide a lower-level connection interface Mar 16, 2021
@SergioBenitez SergioBenitez reopened this Jun 30, 2021
@SergioBenitez SergioBenitez added the enhancement A minor feature request label Jul 1, 2021
@toonvd
Copy link

toonvd commented Jul 27, 2021

This is awesome, one thought, the socket connection should be available so we can usegetsockopt on it since it can be useful for using SO_PEERCRED for example. I don't know if this would be possible using an upstream crate. At the moment I am missing this possibility in some other Rust frameworks / implementations where the socket connection is not usable in hooks for example.

@ibraheemdev
Copy link

@jcdickinson poll_accept can be implemented easily with poll_readable and reading from the underlying std::TcpListener.

@SergioBenitez
Copy link
Member

I'd really like to land something like this soon. From #1820 (comment):

I don't think exposing hyper types is the approach we want to take on this one. I think the approach we want to take is one significantly less heavy-handed. Something like:

rocket::build()
    .listen(anything_implementing_listener)

One of the issues with doing this before was configuration. Given the monumental improvements in configuration, I think we can now say that the proper approach here would be to allow the listener to configure itself. The default TCP/TLS listener would read values as it does today, but otherwise the listener can do whatever it wants.

This is effectively the "user-driven" approach with an answer for configuration. Rocket would give you a default listener, which at the moment only knows about TCP/TLS but can learn about unix domain sockets and so on. A user, however, can implement any kind of listener they want.

@SergioBenitez
Copy link
Member

Here are some more details for the kind of resolution of this issue that I'd like to see, copied from #2013 (comment):

  • Remove all connection configuration from the top-level Config.
  • Modify or create a composable Listener trait that knows how to:
    • Extract its configuration from the configured provider.
    • Set itself up and provide something for Rocket to read data from.
    • Emit an "address" in its "native" format.
  • Implement Listener for what Rocket does internally today with TCP/TLS.
  • Implement Listener for Unix Domain Sockets.
  • Implement Listener for some default listener that composes that previous two.
  • Add a builder method to Rocket to set a different Listener.
  • Set the default listener to be the composed listener from the previous point.

@eirnym
Copy link

eirnym commented Nov 18, 2023

Any news on this?

@jeroenhabets
Copy link

Any news is appreciated, as I am eagerly awaiting UNIX socket support in some downstream projects (on Linux, not interested in Windows)

@SergioBenitez
Copy link
Member

I'll be tackling this later this week.

@SergioBenitez
Copy link
Member

SergioBenitez commented Dec 19, 2023

Currently blocked on #2671, which I'm now prioritizing to make working on this possible.

@github-project-automation github-project-automation bot moved this from In progress to Done in Rocket v0.6 Jan 30, 2024
@SergioBenitez
Copy link
Member

This is now in master. To use unix domain sockets, simply set an address with a unix: prefix and the desired path to the socket following. For example:

ROCKET_ADDRESS=unix:/tmp/rocket.sock cargo run

Or in Rocket.toml:

address="unix:path/to/some/socket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A minor feature request
Projects
Status: Done
Development

No branches or pull requests

8 participants