Client feature proposal: set_local_address

[kw217]

Hi,

I am using the Hyper client on a host which has multiple network interfaces. I would like to issue an HTTP request over a specific interface. The syntax should be something like this:

    let bind_addr: Option<IpAddr> = Some("127.0.0.1".parse().unwrap());
    let client = Client::configure().set_local_address(bind_addr).build(&handle);

Below I propose:

• A configuration syntax.
• A way of implementing this architecturally, which involves changes to mio and tokio-core.

Would you be happy to receive a PR along these lines, and do you think the approach of enhancing mio and tokio-core is best, or should we simply change hyper?

Details follow.

Uncontroversially we need the following plumbing:

• in client/mod.rs we add local_address: Option<IpAddr> to Config, default it to None, add set_local_address, and make build set it on HttpConnector if present.
• Similarly, we add local_address: Option<IpAddr> to the fields of HttpConnector and provide a setter.
• To convey the local address to the point where the connection is done, the Lazy and Resolving states of HttpConnecting gain an extra tuple field Option<IpAddr>, and ConnectingTcp gains a local_address field. The same will also be required in the hyper-tls connector.

The interesting part is what we replace self.current = tokio::net::TcpStream::connect(&addr, handle); with in ConnectingTcp.

Doing it all purely in Hyper looks like this (replacing a few lines of impl ConnectingTcp around https://github.com/hyperium/hyper/blob/master/src/client/connect.rs#L426):

              if let Some(addr) = self.addrs.next() {
                debug!("connecting to {}", addr);
                let do_bind = |local: &Option<IpAddr>| {
                    let sock = match addr {
                        SocketAddr::V4(..) => TcpBuilder::new_v4(),
                        SocketAddr::V6(..) => TcpBuilder::new_v6(),
                    }?;
                    match local {
                        &None => (), // oops, should add special Windows knowledge here
                        &Some(ref local_addr) => sock.bind(SocketAddr::new(local_addr.clone(), 0)).map(|_| ())?,
                    };
                    sock.to_tcp_stream()
                };

                match do_bind(&self.local_address) {
                    Ok(tcp) => {
                        self.current = Some(mio::net::TcpStream::connect_stream(tcp, &addr, handle));
                        continue;
                    },
                    Err(e) => {
                        err = Some(e)
                    }
                };
            }

This direct approach is unpleasant because it violates the architectural layering and duplicates code. In particular, this code is taken almost verbatim from mio::net::TcpStream::connect() – and as you can see there, it should really include a special Windows workaround here.

It also loses the zero-allocation property: Tokio’s TcpStream::connect_stream can only return a Box<Future>, so we have to change ConnectingTcp to use this boxed future rather than the bare Tokio TcpStreamNew (which we cannot construct outside Tokio).

How could we preserve the architectural layering?

In mio::net::TcpStream we should add a connect_with_local_address method beside connect, to encapsulate the Windows and socket knowledge. We should then expose that in a new tokio::net::TcpStream::connect_with_local_address method. Then this method is available to be used Hyper’s ConnectingTcp:

              if let Some(addr) = self.addrs.next() {
                debug!("connecting to {}", addr);
                self.current = tokio::net::TcpStream::connect_with_local_address(&addr, &local, handle);

I think this a cleaner approach architecturally, and it is also able to preserve zero-allocation. But it does involve making changes to three projects. Which is the best approach?

[seanmonstar]

Hey, thanks for the detailed write-up! It really helps me to understand what the end goal is. In this case, it seems like a fine feature for an advanced configuration to allow binding to a local address before connecting to a remote. Some jumbled thoughts:

• I'd probably suggest that it should be an option when configuring an HttpConnector, and not the Client directly, since it doesn't make sense in a generic fashion.
• It seems we can use TcpStream::connect_std, which doesn't return a boxed future. So, there shouldn't be need to make changes to mio and tokio, right?

[kw217]

Thanks @seanmonstar. Yes, making it an option on the connector makes lots of sense, so it looks like this:

let bind_addr: Option<IpAddr> = Some("127.0.0.1".parse().unwrap());
let mut connector = HttpConnector::new(4);
connector.set_local_address(bind_addr);
let client = Client::builder().build(connector);

Aha, I see now that you've upgraded away from tokio-core (I was looking at the latest 0.11 release, not master). It looks a bit simpler now, and I see the Windows knowledge is already in Hyper so the need for it is no longer a concern. Thanks!

However there is one remaining problem - where previously I had to allocate, now I think there's a case that isn't expressible at all. hyper::client::http::connect() takes an Option<Handle>, and I can extend it to also take a local_address: Option<IpAddr>. If the handle is present, I can bind to the local address before calling TcpStream::connect_std. But if the handle is absent, the only method I can call is TcpStream::connect - and that doesn't provide any way to bind first. Am I safe to call just Handle::current() here and call TcpStream::connect_std anyway, or is something missing from tokio?

[seanmonstar]

Yes, calling Handle::current() at that point is exactly what TcpStream::connect(addr) does internally, so that's fine. The point is just to call it directly before needing it, instead of calling Handle::current() when the HttpConnector is created, since at that point, the reactor may not be setup yet.