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

Decouple network stack from types and implementation #434

Open
soundofspace opened this issue Feb 26, 2025 · 1 comment
Open

Decouple network stack from types and implementation #434

soundofspace opened this issue Feb 26, 2025 · 1 comment
Assignees
Labels
needs input question Further information is requested

Comments

@soundofspace
Copy link
Collaborator

While working on #107 I noticed that EstablishedClientConnection includes the socket used for the connection. While in most cases a connection will indeed use a socket, it also doesn't in a lot of other cases. Some examples are:

  • Tests specific layers/services that never go over the network
  • Unix pipes
  • Files
  • Generic connections eg component to each other...

I also remember this being a problem in some other places of the codebase, but will mention these later here when I come across them again.
I believe that types and services should know/need as little as possible from other layers. This makes them very easy to test and this also aligns with how the internet was designed (mostly): layers stacked on top of each other and that can be swapped however you want.

For addr here specifically I see two options currently without doing more research:

  • Put it in context as extension similar to negotiated tls params
  • Put in in connections wrapper type for layers that actually make this socket (eg TcpConnector) and deref everything to the nested connection by default to make it work transparently
  • (make addr optional, but this feels like a bandaid solution)

Right now I prefer the ctx approach, but I might be missing some crucial details here

@GlenDC GlenDC added question Further information is requested needs input blocked Tasks which are blocked on other work. labels Feb 26, 2025
@GlenDC
Copy link
Member

GlenDC commented Feb 26, 2025

Hi @soundofspace , thank you for raising this issue. You bring home some valid points.
I've reviewed the rama codebase and seems like we do not really care about this socketAddress, if I get you right what you would want is that EstablishClientConnection becomes like this:

/// The established connection to a server returned for the http client to be used.
pub struct EstablishedClientConnection<S, State, Request> {
    /// The [`Context`] of the `Request` for which a connection was established.
    pub ctx: Context<State>,
    /// The `Request` for which a connection was established.
    pub req: Request,
    /// The established connection stream/service/... to the server.
    pub conn: S,
}

This seems to be possible with no friction, and 95% of the time we are just passing this to the next layer.
The only place where I could find it being used was in rama-haproxy/src/client/layer.rs

However that is no friction either as you can quite simply, as far as I can see, to change such haprproxy impl signatures like:

impl<S, P, State, Request> Service<State, Request> for HaProxyService<S, P, version::One>
where
    S: ConnectorService<State, Request, Connection: Stream + Unpin, Error: Into<BoxError>>,
    P: Send + 'static,
    State: Clone + Send + Sync + 'static,
    Request: Send + 'static,
{

Instead of Connection: Stream + Unpin to define it like Connection: Stream + Socket + Unpin

Where Socket is https://docs.rs/rama/latest/rama/net/stream/trait.Socket.html

As anyway you only ever expect haproxy to be implemented over something like a TcpConnector, which produces a TcpStream that supports that trait.

Context I try to avoid as much as possible as it makes it a non-typed solution. It's a great escape hatch but in general if we can get away with decent trait bounds that is the best. And unless I miss something here, that is very much possible here.

The PR should be relatively simple to review and ship. It will be big in line count, but 95% of the lines will be just that addr, line deleted :P

@GlenDC GlenDC removed the blocked Tasks which are blocked on other work. label Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs input question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants