bytebeamio / rumqtt

The MQTT ecosystem in rust
Apache License 2.0
1.53k stars 234 forks source link

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

Open caoruijuan13 opened 3 months ago

caoruijuan13 commented 3 months ago

Type of change

Checklist:

swanandx commented 3 months ago

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

caoruijuan13 commented 2 months ago

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 commented 2 months ago

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.

cavivie commented 1 month ago

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

swanandx commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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);