extensible entrypoint for app.listen #610

jbr · 2020-06-21T00:20:38Z

The idea is that it should be possible for a third-party crate to implement tide::listener::ToListener and tide::listener::Listener for any newtype (eg multiple varieties of TlsListener) and support passing it to Server::listen and adding it to a MultiListener alongside other Listeners.

Working experimental example of tls on tide: https://github.com/jbr/tide-tls

Note for future changes:

In order to provide hard/soft concurrency limits on open requests as per #586, the Listener trait will likely need to change to include a hook of some sort for backpressure, but that was beyond the scope of this already-large PR. Similarly, the Listener trait would be a good place to introduce a graceful shutdown mechanism

src/listener/tcp_listener.rs

src/listener/to_listener.rs

Fishrock123 · 2020-06-24T21:34:33Z

Overall this seems like a good direction to me.

yoshuawuyts · 2020-06-26T14:17:33Z

examples/multi_listener.rs

+        ))
+    });
+
+    app.listen(vec!["localhost:8000", "localhost:8081"]).await?;


I would interpret this as: "try listening on port 8000 first, if that's not possible run on port 8081". This is how ToSocketAddr for [SocketAddr] works in std as well.

Personally, that seems like a surprising behavior, try_listen seems like a better name for that but I suppose that could already be idiomatic in std rust.

I started writing an implementation of the fallback port multi-listener, but I realized I don't understand the use case. When ToSocketAddr resolves to multiple ports, those ports represent different notions of the same thing, so like binding to [::1]:8000 vs 127.0.0.1:8000 are both representing the notion of "localhost:8000" so a system could reliably provide "localhost:8000" and then access it from port 8000 by reverse proxying to that port, for example. However, providing a vec of ["localhost:8000", "localhost:8001", "localhost:8002"] does not represent one resource, and I don't understand how the behavior of fallback ports would actually be used. If 8000 is currently bound, isn't that a concerning state that should halt app startup?

In comparison, binding to multiple ports concurrently has several slightly-more-common use cases, such as supporting both uds for fast clients (local to the machine) also to tcp for network clients, or binding to both a public network interface as well as a private network interface, or potentially binding to both tcp://0.0.0.0:80 and tls://0.0.0.0:443 with appropriate permissions (authbind, etc).

yoshuawuyts · 2020-06-26T14:17:40Z

src/lib.rs

-//! [`Response::set_ext`] and is available through [`Response::ext`].
+//! [`Response::insert_ext`] and is available through [`Response::ext`].


Nice catch!

src/listener/mod.rs

src/listener/parsed_listener.rs

yoshuawuyts · 2020-06-26T14:48:00Z

src/listener/multi_listener.rs

+///```
+
+#[derive(Default)]
+pub struct MultiListener<State>(Vec<Box<dyn Listener<State>>>);


Something we talked about yesterday was that it'd be nice to find a way to add functionality as "listen on multiple listeners" and "attempt to connect to these listeners in this sequence".

addressed by renaming MultiListener to ConcurrentListener and adding FailoverListener. I don't understand the use case well enough to write good/representative docs or examples, though: https://github.com/http-rs/tide/pull/610/files#diff-92873655898e8cfdba457e044d652af1 Importantly, this sort of thing would be easy for apps to implement on their own, or third party crates, etc. I think the only tide-internal question is what Vec turns into

src/listener/to_listener.rs

yoshuawuyts · 2020-06-26T15:22:12Z

src/listener/tcp_listener.rs

+#[derive(Debug)]
+pub enum TcpListener {
+    FromListener(net::TcpListener),
+    FromAddrs(Vec<SocketAddr>, Option<net::TcpListener>),
+}


Hmm, I guess I'm somewhat confused by this structure. In the stdlib ToSocketAddr is resolved the moment it is created. Is there a practical reason we're not doing the same here?

That way we can store a single async_std::net::TcpListener that we then later listen on. The same goes for unix_listener.rs. ToListener::to_listener is fallible, so I don't believe it should be a problem.

The difference is that ToSocketAddr doesn't immediately bind to that address. In this version of the code, we do as much resolution as we can, but we avoid binding until listen is called. That way this doesn't exhibit confusing behavior:

let mut multi = MultiListener::new(); multi.add("localhost:8000")?; multi.add("unix+http://file")?; // are 8000 and ./file bound here? I wouldn't expect them to be yet multi.listen().await?; //but now I would expect them to be.

If this is not the case, MultiListener::add is actually MultiListener::listen which would be a different approach. In this design, there's a state distinction between a Listener that can listen and a bound transport

I also would not expect resources by string to be opened until listen is called, unless someone passes in something that is already explicitly bound, i.e. a struct for a file descriptor or something.

src/listener/to_listener.rs

yoshuawuyts · 2020-06-26T15:25:28Z

src/listener/to_listener.rs

+impl<State: Send + Sync + 'static> ToListener<State> for ParsedListener {
+    type Listener = Self;
+    fn to_listener(self) -> io::Result<Self::Listener> {
+        Ok(self)
+    }
+}


What does parsedlistener do?

This was a simple approach to wrapping two specific listener types without having to make them dyn. We can only parse a limited number of types currently, so this is the associated Listener type that's returned from string parsing

yoshuawuyts · 2020-06-26T15:34:31Z

src/listener/to_listener.rs

+pub trait ToListener<State: Send + Sync + 'static> {
+    type Listener: Listener<State>;
+    /// Transform self into a
+    /// [`Listener`](crate::listener::Listener). Unless self is
+    /// already bound/connected to the underlying io, converting to a
+    /// listener does not initiate a connection. An Err return
+    /// indicates an unsuccessful conversion to a listener, not an
+    /// unsuccessful bind attempt.
+    fn to_listener(self) -> io::Result<Self::Listener>;
+}


One question that I have is: why don't we perform the listening logic inside of the to_listener function's body? Intuitively it feels like we may be having one layer of traits more than is required. We can still reuse logic by having an e.g. listen_unix function that knows how to listen on a unix socket, but that can be called from this function.

I guess what I'm trying to say is that I'm unclear why "convert into a listener" and "listen on said listener" are two separate steps. My guess is that it's probably coming from the desire to have MultiListener?

During our chat yesterday I showed how to manually MultiListener's functionality using task::spawn and future::join. We agreed that doing this manually isn't ideal, so having an intermediate abstraction makes sense. But what I think that shows is that servers that have been started can in fact be combined after the fact.

I'm not entirely convinced that I've quite covered all the details here -- but I feel like there might be something to it. Would be keen to hear your thoughts on this!

I made an experimental branch at https://github.com/jbr/tide/tree/simpler-listener-trait for comparison. Docs haven't been updated, but: There's no longer a ToListener trait, just a Listener trait. Calling Listener::<State>::listen("http://localhost:8000").await immediately binds, and Listener is implemented for all of the types that ToListener had been implemented for. MultiListener also binds immediately, though, which feels like a step backwards. I couldn't figure out how to get it to correctly hold the Box<dyn Listener>s. Listen needs to take self because some listeners need to move self into the listener.

I still prefer the two-step version, but it's true that there's slightly less code in the one-trait version: jbr@e86e957

Cargo.toml

yoshuawuyts

LGTM!

and emit multiple logs

jbr added the enhancement New feature or request label Jun 21, 2020

jbr force-pushed the to-listener branch 3 times, most recently from 93f6ccd to 3ab3a72 Compare June 22, 2020 17:14

jbr changed the title ~~WIP: single extensible entrypoint for app.listen~~ single extensible entrypoint for app.listen Jun 24, 2020

jbr changed the title ~~single extensible entrypoint for app.listen~~ extensible entrypoint for app.listen Jun 24, 2020

jbr force-pushed the to-listener branch from 8533d4d to 69702cf Compare June 24, 2020 01:47

jbr marked this pull request as ready for review June 24, 2020 02:52

jbr requested a review from yoshuawuyts June 24, 2020 02:53

jbr force-pushed the to-listener branch from 69702cf to d255605 Compare June 24, 2020 02:55

Fishrock123 reviewed Jun 24, 2020

View reviewed changes

src/listener/tcp_listener.rs Show resolved Hide resolved

src/listener/to_listener.rs Show resolved Hide resolved

src/listener/to_listener.rs Show resolved Hide resolved

jbr force-pushed the to-listener branch 2 times, most recently from f39de2a to 089e996 Compare June 25, 2020 21:42

yoshuawuyts reviewed Jun 26, 2020

View reviewed changes

Fishrock123 reviewed Jul 9, 2020

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

yoshuawuyts approved these changes Jul 9, 2020

View reviewed changes

jbr force-pushed the to-listener branch from 7a609b6 to ade84f2 Compare July 11, 2020 01:49

jbr added 12 commits July 13, 2020 15:12

Introduce ToListener and Listener

59b09d3

remove connect from the Listener trait

fd0d2b2

and emit multiple logs

rename MultiListener::with to with_listener

4fc46f2

clippied

09b65da

doc fix

bd19ca8

simplify MultiListeners with FuturesUnordered

d07585b

clean up &str ToListener

492b0bf

prefer http+unix

18fad44

rename MultiListener to ConcurrentListener

37df3ae

add failover listener for discussion

30c1ac0

*only* support http+unix://

80b283d

update "server listening" message

c161c2d

jbr added 2 commits July 13, 2020 15:14

use async_trait

9977c68

add some docs for internal-only listeners

1603404

jbr force-pushed the to-listener branch from 8cb5707 to 1603404 Compare July 13, 2020 23:40

jbr merged commit ba65ca4 into http-rs:master Jul 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extensible entrypoint for app.listen #610

extensible entrypoint for app.listen #610

jbr commented Jun 21, 2020 •

edited

Loading

Fishrock123 commented Jun 24, 2020

yoshuawuyts Jun 26, 2020

Fishrock123 Jun 26, 2020

jbr Jun 27, 2020

yoshuawuyts Jun 26, 2020

yoshuawuyts Jun 26, 2020

jbr Jun 27, 2020

yoshuawuyts Jun 26, 2020

jbr Jun 26, 2020 •

edited

Loading

Fishrock123 Jun 26, 2020

yoshuawuyts Jun 26, 2020

jbr Jun 26, 2020

yoshuawuyts Jun 26, 2020

jbr Jun 27, 2020

yoshuawuyts left a comment

		//! [`Response::set_ext`] and is available through [`Response::ext`].
		//! [`Response::insert_ext`] and is available through [`Response::ext`].

extensible entrypoint for app.listen #610

extensible entrypoint for app.listen #610

Conversation

jbr commented Jun 21, 2020 • edited Loading

Fishrock123 commented Jun 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbr Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts left a comment

Choose a reason for hiding this comment

jbr commented Jun 21, 2020 •

edited

Loading

jbr Jun 26, 2020 •

edited

Loading