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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rumqttc/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* `size()` method on `Packet` calculates size once serialized.
* `read()` and `write()` methods on `Packet`.
* `ConnectionAborted` variant on `StateError` type to denote abrupt end to a connection
* `bind_addr` for `NetworkOptions` to enable `TCPSocket.bind()`

### Changed

Expand Down
4 changes: 4 additions & 0 deletions rumqttc/src/eventloop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,10 @@ pub(crate) async fn socket_connect(
socket.set_recv_buffer_size(recv_buffer_size).unwrap();
}

if let Some(bind_addr) = network_options.bind_addr {
socket.bind(bind_addr)?;
}

#[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))]
{
if let Some(bind_device) = &network_options.bind_device {
Expand Down
15 changes: 14 additions & 1 deletion rumqttc/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,10 @@
#[macro_use]
extern crate log;

use std::fmt::{self, Debug, Formatter};
use std::{
fmt::{self, Debug, Formatter},
net::{SocketAddr, ToSocketAddrs},
};

#[cfg(any(feature = "use-rustls", feature = "websocket"))]
use std::sync::Arc;
Expand Down Expand Up @@ -370,6 +373,7 @@ pub struct NetworkOptions {
tcp_send_buffer_size: Option<u32>,
tcp_recv_buffer_size: Option<u32>,
conn_timeout: u64,
bind_addr: Option<SocketAddr>,
#[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))]
bind_device: Option<String>,
}
Expand All @@ -380,6 +384,7 @@ impl NetworkOptions {
tcp_send_buffer_size: None,
tcp_recv_buffer_size: None,
conn_timeout: 5,
bind_addr: None,
#[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))]
bind_device: None,
}
Expand All @@ -404,6 +409,14 @@ impl NetworkOptions {
self.conn_timeout
}

/// bind connection to a specific socket address
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

self
}

/// bind connection to a specific network device by name
#[cfg(any(target_os = "android", target_os = "fuchsia", target_os = "linux"))]
#[cfg_attr(
Expand Down
Loading