Skip to content

net-utils: introduce new helper functions #7055

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

Merged
alexpyattaev merged 2 commits into from
Jul 24, 2025
Merged

net-utils: introduce new helper functions #7055

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56aa2d4
port helper functions
puhtaytow Jul 21, 2025
8d86cec
move Ipv4Addr import to global scope
puhtaytow Jul 21, 2025
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
39 changes: 30 additions & 9 deletions net-utils/src/sockets.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,21 @@
#[cfg(feature = "dev-context-only-utils")]
Copy link
Copy Markdown

@alexpyattaev alexpyattaev Jul 21, 2025

Choose a reason for hiding this comment

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

Its really odd that this one got migrated up...

Copy link
Copy Markdown
Author

@puhtaytow puhtaytow Jul 21, 2025

Choose a reason for hiding this comment

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

Auto-formatting it is. Do you want me to bring it back to the previous location?

Copy link
Copy Markdown

@alexpyattaev alexpyattaev Jul 21, 2025

Choose a reason for hiding this comment

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

nope, it does not matter just odd.

use tokio::net::UdpSocket as TokioUdpSocket;
use {
crate::PortRange,
log::warn,
socket2::{Domain, SockAddr, Socket, Type},
std::{
io,
net::{IpAddr, SocketAddr, TcpListener, UdpSocket},
net::{IpAddr, Ipv4Addr, SocketAddr, TcpListener, UdpSocket},
ops::Range,
sync::atomic::{AtomicU16, Ordering},
},
};
#[cfg(feature = "dev-context-only-utils")]
use {std::net::Ipv4Addr, tokio::net::UdpSocket as TokioUdpSocket};
// base port for deconflicted allocations
const BASE_PORT: u16 = 5000;
// how much to allocate per individual process.
// we expect to have at most 64 concurrent tests in CI at any moment on a given host.
const SLICE_PER_PROCESS: u16 = (u16::MAX - BASE_PORT) / 64;
/// Retrieve a free 20-port slice for unit tests
///
/// When running under nextest, this will try to provide
/// a unique slice of port numbers (assuming no other nextest processes
/// are running on the same host) based on NEXTEST_TEST_GLOBAL_SLOT variable
Expand All @@ -25,9 +24,9 @@ const SLICE_PER_PROCESS: u16 = (u16::MAX - BASE_PORT) / 64;
/// When running without nextest, this will only bump an atomic and eventually
/// panic when it runs out of port numbers to assign.
#[allow(clippy::arithmetic_side_effects)]
pub fn localhost_port_range_for_tests() -> (u16, u16) {
pub fn unique_port_range_for_tests(size: u16) -> Range<u16> {
Comment thread
KirillLykov marked this conversation as resolved.
static SLICE: AtomicU16 = AtomicU16::new(0);
let offset = SLICE.fetch_add(20, Ordering::Relaxed);
let offset = SLICE.fetch_add(size, Ordering::Relaxed);
let start = offset
+ match std::env::var("NEXTEST_TEST_GLOBAL_SLOT") {
Ok(slot) => {
Expand All @@ -40,8 +39,30 @@ pub fn localhost_port_range_for_tests() -> (u16, u16) {
}
Err(_) => BASE_PORT,
};
assert!(start < u16::MAX - 20, "ran out of port numbers!");
(start, start + 20)
assert!(start < u16::MAX - size, "Ran out of port numbers!");
start..start + size
}

/// Retrieve a free 20-port slice for unit tests
///
/// When running under nextest, this will try to provide
/// a unique slice of port numbers (assuming no other nextest processes
/// are running on the same host) based on NEXTEST_TEST_GLOBAL_SLOT variable
/// The port ranges will be reused following nextest logic.
///
/// When running without nextest, this will only bump an atomic and eventually
/// panic when it runs out of port numbers to assign.
pub fn localhost_port_range_for_tests() -> (u16, u16) {
Copy link
Copy Markdown

@gregcusack gregcusack Jul 22, 2025

Choose a reason for hiding this comment

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

should this also return a Range<u16>? Feels like we should return port ranges in one way, rather than two.

Copy link
Copy Markdown

@alexpyattaev alexpyattaev Jul 22, 2025

Choose a reason for hiding this comment

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

We have a whole bunch of code that expects this port range format. Legacy:)

Copy link
Copy Markdown

@gregcusack gregcusack Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ahh ok sad!

we do have the off by one error here still. Range<u16> is great since it returns inclusive on lower end and exclusive on upper end. But if we explicitly use (pr.start, pr.end), this returns a range of 21 usable ports by the caller. so unique_port_range_for_tests() only reserves 20 ports but localhost_port_range_for_tests() returns 21 ports to the caller. Maybe we can make a note here or something saying that localhost_port_range_for_tests() returns the range, inclusive start, exclusive end. Do not use the end port.

Copy link
Copy Markdown
Author

@puhtaytow puhtaytow Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

You are absolutely right. I was convinced that the fact, that the localhost_port_range_for_tests() would get Range from the unique_port_range_for_tests resolves the issue, but i just checked it in tests and was wrong indeed. Thank you for pointing this issue again.

Since in the tests that base on the localhost_port_range_for_tests we use ports in the range with exclusive end, these getting as expected 20 ports, but i agree it might be used incorrectly.

I scanned thru the repo and there is 35 occurrences of the localhost_port_range_for_tests. If we fix it in this PR it would be quite extensive again, hence why, as you suggested, I'll add the comment and in the next PR, we might either switch to Range in this function as well, or even remove it entirely and use directly the unique_port_range_for_tests

Thank you 🙏

Copy link
Copy Markdown

@alexpyattaev alexpyattaev Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As I understand it, the code as proposed will fix the localhost_port_range_for_tests, so it will work perfectly fine returning 1 less port. No need to change its users. I'd remove the note there, it does nothing but add confusion.

Copy link
Copy Markdown
Author

@puhtaytow puhtaytow Jul 23, 2025

Choose a reason for hiding this comment

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

Fixed. Thank you 🙏

Copy link
Copy Markdown

@gregcusack gregcusack Jul 24, 2025

Choose a reason for hiding this comment

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

sorry i'm late on this. if we call unique_port_range_for_tests(20), we are allocating/reserving 20 ports, say port 10000 -> 10019. But the range returned is [10000, 10020). Which is exactly what we want. But then localhost_port_range_for_tests() will return ports 10000 -> 10020 (inclusive). So if a test binds to 10020 and another test calls unique_port_range_for_tests(20), returning [10020, 10040), we will have a one port overlap (10020), causing a test to fail for the same reason we are trying to fix.

let pr = unique_port_range_for_tests(20);
(pr.start, pr.end)
}

/// Bind a `UdpSocket` to a unique port.
pub fn bind_to_localhost_unique() -> io::Result<UdpSocket> {
bind_to(
IpAddr::V4(Ipv4Addr::LOCALHOST),
unique_port_range_for_tests(1).start,
)
}

pub fn bind_gossip_port_in_range(
Expand Down
Loading