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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

caoruijuan13
Copy link

@caoruijuan13 caoruijuan13 commented Mar 26, 2024

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
Copy link
Member

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

@@ -404,6 +409,11 @@ impl NetworkOptions {
self.conn_timeout
}

pub fn set_bind_addr(&mut self, bind_addr: SocketAddr) -> &mut Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is missing here on a pub fn

@@ -404,6 +409,11 @@ impl NetworkOptions {
self.conn_timeout
}

pub fn set_bind_addr(&mut self, bind_addr: SocketAddr) -> &mut Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Like this?

Copy link
Author

@caoruijuan13 caoruijuan13 Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@caoruijuan13
Copy link
Author

caoruijuan13 commented Apr 8, 2024

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
Copy link

cavivie commented Apr 8, 2024

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.

Copy link
Member

@swanandx swanandx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for contributing!

Please do the needful for v5 client as well. 😁

pub fn set_bind_addr(&mut self, bind_addr: impl ToSocketAddrs) -> &mut Self {
self.bind_addr = bind_addr
.to_socket_addrs()
.map_or(None, |mut iter| iter.next());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@cavivie cavivie Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(())
}

Copy link
Author

@caoruijuan13 caoruijuan13 Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@caoruijuan13 caoruijuan13 Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@cavivie cavivie Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@caoruijuan13 caoruijuan13 Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cavivie
Copy link

cavivie commented May 8, 2024

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

@swanandx
Copy link
Member

swanandx commented May 8, 2024

@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
Copy link

cavivie commented May 8, 2024

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
Copy link
Member

swanandx commented May 8, 2024

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
Copy link

cavivie commented May 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants