{{ message }}
Fix(tests): replace hardcoded stratum ports with OS-assigned port 0#477
Merged
Conversation
Contributor
zaidmstrr
reviewed
Jun 10, 2026
zaidmstrr
left a comment
zaidmstrr
left a comment
Contributor
There was a problem hiding this comment.
Tested the PR, most of the things look good. But in the future we can perform a good refactor as a follow-up PR, which can enhance the code readability and provide a nicer architecture. By moving the TcpListener::bind call out of the run_stratum_service and into the caller. Here, the caller binds port 0 itself; the server function just accepts the ready listener, not a port.
Tests binding fixed ports (3353, 3356, 3357, 3358, 5050) caused AddrInUse failures when tests ran concurrently or a previous test left a server listening. This was a known flake documented across multiple PRs (braidpool#309, braidpool#474). Add bound_addr_tx: Option<oneshot::Sender<SocketAddr>> to run_stratum_service. After binding, the actual address is sent through the channel so tests can connect to the real port. All 5 tests now use port 0 (OS-assigned) and await the oneshot receiver before connecting, no sleep needed, no race window. Production call in main.rs passes None.
server_endpoints and the warning log were using self.stratum_config.port which shows 0 when OS port assignment is used. Switch to bound_addr.port() so logs always show the real listening port.
nkatha23
force-pushed
the
fix/test-hardcoded-ports
branch
from
17a64a7 to
4207ad8
Compare
nkatha23
added a commit
to nkatha23/braidpool
that referenced
this pull request
Jun 15, 2026
run_stratum_service now accepts a ready TcpListener instead of binding internally. The caller (main.rs or tests) binds the socket and passes it in, removing the need for the oneshot channel workaround introduced in PR braidpool#477. This separates concerns cleanly — the function runs the server,the caller controls how the socket is acquired. Consistent with how rpc_server.rs handles its server socket via local_addr(). Changes: - run_stratum_service: listener: TcpListener replaces bound_addr_tx - main.rs: binds stratum_listener before spawn, passes it in - All 5 test sites: bind port 0, read local_addr() directly - Remove oneshot from stratum.rs imports
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While reviewing PRs #309 and #474, I kept seeing test_invalid_json fail with AddrInUse on port 5050. Looking into it, all five stratum tests bind fixed ports (3353, 3356, 3357, 3358, 5050), if a previous test leaves a
server running or two tests race to bind the same port, the test fails with a misleading error that looks like a code bug rather than a port conflict.
The fix adds an optional oneshot channel parameter to run_stratum_service. After binding, the server sends its actual address back through the channel. Tests use port 0 so the OS assigns a free port, then await the receiver before connecting with no sleep, race windows, or hardcoded ports.
rpc_server.rs already uses the same pattern via server.local_addr(), so this is consistent with how the rest of the codebase handles this.
Production call in main.rs passes None, zero impact outside tests.