streamer, tests: use the non-overlapping socket bind api

[puhtaytow]

Problem

A bunch of tests still relies on either hardcoded ports or bind_to_localhost and bind_to_unspecified. This creates flaky tests in cases where unique port ranges are needed.

Related to #6886

Summary of Changes

Swapped to the new localhost_port_range_for_tests.

[0xbrw]

@alexpyattaev or @apfitzge when you get a chance, can I have you take a look?

[apfitzge]

It looks correct to me on skim, but it's a bit verbose to do this everywhere. Can we not have a bind_to_localhost_for_tests that just accesses some static behind a mutex that is lazily initialized with this iterator? That'd make a lot of these simple function changes instead of having like 4-5 extra lines per change.

[puhtaytow]

It looks correct to me on skim, but it's a bit verbose to do this everywhere. Can we not have a bind_to_localhost_for_tests that just accesses some static behind a mutex that is lazily initialized with this iterator? That'd make a lot of these simple function changes instead of having like 4-5 extra lines per change.

I looked at the https://github.com/anza-xyz/agave/blob/master/net-utils/src/sockets.rs#L27-L45 and to me, the function does utilize the nextest env variable to coordinate the tests globally, there is also the atomic SLICE.. it looks like we could skip mutex in this case.

Obviously, correct me if im wrong 🙏

pub fn localhost_port_range_for_tests() -> (u16, u16) {
    static SLICE: AtomicU16 = AtomicU16::new(0);
    let offset = SLICE.fetch_add(20, Ordering::Relaxed);
    let start = offset
        + match std::env::var("NEXTEST_TEST_GLOBAL_SLOT") {
            Ok(slot) => {
                let slot: u16 = slot.parse().unwrap();
                assert!(
                    offset < SLICE_PER_PROCESS,
                    "Overrunning into the port range of another test! Consider using fewer ports per test."
                );
                BASE_PORT + slot * SLICE_PER_PROCESS
            }
            Err(_) => BASE_PORT,
        };
    assert!(start < u16::MAX - 20, "ran out of port numbers!");
    (start, start + 20)
}

To address the unnecessary verbosity, i introduced the helper function in net-utils as per your suggestion. https://github.com/anza-xyz/agave/compare/master...puhtaytow:agave:streamer-non-overlapping-socket-bind-api-1?expand=1

Please tell me if that is the correct place for it and if so then how would you like to merge the changes to net-utils, as separate PR?

Thank you for the suggestion 🙏 i do appreciate a lot any constructive input.

[alexpyattaev]

Please do not fix what is not broken =) localhost_port_range_for_tests uses both nextest env variables AND local atomics to coordinate port allocation because we do not know if it will run under cargo test or cargo nextest. cargo test uses 1 process for all tests => atomics work. cargo nextest uses process-per-test model, and thus we need to use env variables. One possible improvement for that function was suggested by t-nelson that would use shared memory instead of env variables for inter-process coordination, as that would allow the same mechanism to be used under both test runners.

[alexpyattaev]

Let us not create mutable iterators where direct access would achieve the same result=)

[puhtaytow]

@alexpyattaev thank you for pointing these out / fixed 🙏

[alexpyattaev]

LGTM thank you!

[puhtaytow]

LGTM thank you!

Thank you! 🙏

[alexpyattaev]

This is currently blocked by #6957