Feature http transport #296
Conversation
ii64
force-pushed
the
feature/http-transport
branch
from
3f833ad
to
07bef72
Compare
|
The CI failed on building |
| @@ -32,6 +33,7 @@ pub enum ConnStream { | |||
| Rustls(#[pin] tokio_rustls::TlsStream<TcpStream>), | |||
| #[cfg(feature = "native-tls")] | |||
| NativeTls(#[pin] tokio_native_tls::TlsStream<TcpStream>), | |||
| Http(#[pin] Http), | |||
There was a problem hiding this comment.
Is it possible to move the Conn/IO part of volo-http here? @wfly1998
There was a problem hiding this comment.
It'd be nice if you can touch up the implementation to use volo-http, I have no idea about how we can use that with existing volo-http implementation.
Also I wonder of we can reuse the service handler on top of another HTTP server implementation, e.g. Axum, Actix, Rocket, etc etc. So that we can provide multiple service running on the same server for the usecase like /api/authn/v1 will be handled by the AuthenticationServiceV1, /api/feed/v1 will be handled by the FeedServiceV1 etc etc.
.github/workflows/ci.yaml
Outdated
| @@ -33,7 +33,9 @@ jobs: | |||
| # - uses: Swatinem/rust-cache@v1 | |||
| - name: Run tests | |||
| run: | | |||
| apt install -y pkg-config libssl-dev | |||
There was a problem hiding this comment.
Try using vendored for openssl-sys?
There was a problem hiding this comment.
I am not really sure how we can set a transitive dependency features, it's a dependency of tokio-native-tls
There was a problem hiding this comment.
tokio-native-tlshas a featurevendoredfor enablingnative-tls/vendorednative-tlshas a featurevendoredfor enablingopenssl/vendoredopensslhas a featurevendoredfor enablingffi/vendored, and note that theffiisopenssl-sysin the same repositoryopenssl-syshas a featurevendoredthrough whichopensslcan use its ownopensslimplementation instead oflibssl-dev
It's complicated and I don't like them 🥲 but it works.
There was a problem hiding this comment.
But actually with that, we can let the user decide if they want to use vendored (and statically link the library) or just go by the system package by adding our:
[features]
vendored = ["tokio-native-tls/vendored"]What do you think?
There was a problem hiding this comment.
I think adding a feature vendored to enable tokio-native-tls/vendored is the best way, users can decide by themselves, and we can use vendored in CI.
There was a problem hiding this comment.
Sounds good, ref for that change @ ac00381
There was a problem hiding this comment.
Nevermind, I'll open a new PR for that
| pub use incoming::{DefaultIncoming, MakeIncoming}; | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq, Hash)] | ||
| pub enum Address { | ||
| Ip(std::net::SocketAddr), | ||
| #[cfg(target_family = "unix")] | ||
| Unix(Cow<'static, Path>), | ||
| Http(hyper::Uri) |
There was a problem hiding this comment.
Consider that Uri can be a full uri or a relative uri, e.g., https://www.rust-lang.org/install.html or /hello/world, which is not an address.
Additionally, an http or https host can resolve to SocketAddr, or an openapi server can serve on a unix socket, both of which are supported.
I think using Uri as address is not a good idea.
There was a problem hiding this comment.
What I have been thinking about that is, we can play with Uri scheme as like what the gRPC one currently does, e.g. unix:///var/run/some.socket.
So wrap it up for client role, Unix("/var/run/some.socket") will talk to the UNIX domain socket as like TCP one, while Http("unix:///var/run/some.socket") will talk to the UNIX domain socket in HTTP with write-buffered RPC payload as the request body. (Or say that if we want to support bi-directional Thrift, we can use Transfer-Encoding: chunked first as initial connection upgrade and go as like what we did on TCP).
For the relative uri, I am not really sure if we can restrict it, if not in the runtime, because that basically has no scheme so we can just throw a panic when we check it.
And for HTTP or HTTPS hosts that can resolve to SocketAddr, I think we can still use Http("https://127.0.0.1:9999/") whereas 120.0.0.1:9999 is a valid SocketAddr with / added for specified endpoint resource.
Or do you have any idea about it?
There was a problem hiding this comment.
This type is Address, which is the address used for the transport layer of the network model, while HTTP is a protocol of application layer. They should not be confused.
Motivation
Follow up for #125
Solution
RFC, API stabilization. Initial POC is using
reqwest, howevervolohave their ownhttplibrary wrapper, perhaps looking to that later?