feat: add bind_addr for NetworkOptions to enable TCPSocket.bind()

[caoruijuan13]

Type of change

Checklist:

• Formatted with cargo fmt
• Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

[swanandx]

hey @caoruijuan13 , thanks for the contribution, can you please elaborate more on why this is required?

[flxo]

Comment is missing here on a pub fn

[flxo]

What about using impl std::net::ToSocketAddrs?

[cavivie]

What about using impl std::net::ToSocketAddrs?

Like this?

[caoruijuan13]

Cool, impl std::net::ToSocketAddrs is very useful and convenient.

[caoruijuan13]

hey @caoruijuan13 , thanks for the contribution, can you please elaborate more on why this is required?

In my actual use, I need to bind specific address(the default interface address usually, maybe x.x.x.x:0) for the client to avoid routing loop, maybe others need it too.

[cavivie]

Fortunately I took another look at the PR list, otherwise I would have done this again.

Our company now has a multi-platform and multi network devices scenario test. This will cause the bind device to be unable to bind specific address routing in scenarios such as macOS/iOS/Windows. Since the routing characteristics of different platforms are different, it is difficult to use simple routing. Control is achieved by simply binding addresses for rumqtt to avoid re-routing decisions to achieve the same effect, which is really awesome and we need it too.

[swanandx]

Thank you so much for contributing!

Please do the needful for v5 client as well. 😁

[swanandx]

is it fine to just ignore if there was any error in converting to socket address?

Someone can mistakenly supply invalid addr and think it's working just fine.

for now, as there is no mechanism for returning error here, maybe we can just panic or simply ignore just as we do rn.

which one would you prefer?

[cavivie]

If it may cause an error, then I think we should not hide the error, either panic or throw. In this case, I think we can simply throw an error since there is no need to support too much chain calls (generally on like this options).

/// bind connection to a specific socket address
pub fn set_bind_address(&mut self, bind_address: ToSocketAddrs) -> std::io::Result<()> {
    self.bind_address = bind_address.to_socket_addrs()?.next();
    Ok(())
}

[caoruijuan13]

I have also considered this issue and feel that return Result is better.

But set_bind_device is just like this(no Result), so no Result in order to keep consistent.

What do you think?

[caoruijuan13]

And currently v5 is supported, they all use rumqttc::NetworkOptions option, but v5 client set it by MqttOptions, and client set it by Eventloop, just like:

    let mut mqtt_options_v5 = rumqttc::v5::MqttOptions::parse_url(url.clone()).unwrap();
    let mut network_options = rumqttc::NetworkOptions::new();
    network_options.set_bind_addr(*crate::app::addrs::LOCAL_ADDR);
    mqtt_options_v5.set_network_options(network_options);

[cavivie]

Since I'm not sure whether to return an error or mask an error, I think we shouldn't wrap the error behavior too much, so I think we should not use ToSocketAddrs, and this conversion should be decided by the caller (because we are not a low-level crate)

// manually import this trait and convert to socket addr by caller.
use std::net::ToSocketAddrs;
let addr = "127.0.0.1"
let addr = addr.to_socket_addrs().unwrap();

// set network options
network_options
  .set_xxx(xxx)
  . ...
  .set_bind_addr(addr);

[caoruijuan13]

Yes, it wouldn't be a problem if we used SocketAddr instead of ToSocketAddrs.

[cavivie]

@flxo @swanandx I wonder what's stopping us from going further now?

[swanandx]

@flxo @swanandx I wonder what's stopping us from going further now?

same changes needs to be done for v5 client as well.
And i think the discussion about what to do incase it fails to convert to sockeraddr isn't resolved right?

[cavivie]

same changes needs to be done for v5 client as well.

The v5 client is already supported because v5 MqttOptions NetworkOptions references the already implemented lib NetworkOptions, doesn't it?

And i think the discussion about what to do incase it fails to convert to sockeraddr isn't resolved right?

As I said before, we use SocketAddr directly instead of ToSocksAddr, isn't this solution enough to solve this problem?
I think the conversion is handled by the caller rather than in this non-low-level crate, what you think so?

UDP: If you agree with all of these, then we push this PR author change to go further.

[swanandx]

The v5 client is already supported because v5 MqttOptions NetworkOptions references the already implemented lib NetworkOptions, doesn't it?

that is bind_device which is different from bind_addr right? if not, bind_device already exists for v4 client, so we don't even need anything.

I think the conversion is handled by the caller rather than in this non-low-level crate, what you think so?

didn't get it, wdym?

[cavivie]

that is bind_device which is different from bind_addr right? if not, bind_device already exists for v4 client, so we don't even need anything.

bind_addr is different from bind_device.
bind_addr: Bind a socket to a socket address like "192.168.128.1:8080".
bind_device: Bind a socket to a particular device like "eth0", as specified in the passed interface name.

Currently v5 and non-v5 share the same NetworkOptions struct and eventloop socket_connect function, it only needs to be implemented in one place, and the other place will be automatically implemented. For non-v5, just set NetworkOptions directly on MqttOptions, but for v5, need to create NetworkOptions and set it into the EventLoop struct.

If it’s still unclear, I have a PR here to make v5 and non-v5 network options consistent in the way they are used externally.

didn't get it, wdym?

Use the (recommend) way:

/// callers can call `set_bind_address(addr: SocketAddr)`;
///
/// bind connection to a specific socket address
pub fn set_bind_address(&mut self, bind_address: SocketAddr) -> &mut Self {
    self.bind_address = Some(bind_address);
    self
}

Instead of the way(unrecommended):

/// callers can call `set_bind_address(addr: ToSocketAddrs)`.
/// example: set_bind_address("192.168.227.1:8086").
///
/// bind connection to a specific socket address
pub fn set_bind_address(&mut self, bind_address: ToSocketAddrs) -> std::io::Result<()> {
    self.bind_address = bind_address.to_socket_addrs()?.next();
    Ok(())
}

Usage for callers:

/// address convert errors should be handled by callers themself, 
/// not by the rumqttc library, this example just unwrap (panic) it.
use std::net::ToSocketAddrs;
let addr = "192.168.227.1:8086"
let addr: SocketAddr = addr.to_socket_addrs().unwrap();

/// use SocketAddr variable to call `set_bind_address`,
/// won't use `set_bind_address("192.168.227.1:8086")`.
network_options.set_bind_address(addr);