Try every address candidate when binding and when sending a datagram - #14
Conversation
resolve_address took the first addrinfo getaddrinfo returned and freed the rest, so all three call sites inherited whatever the resolver happened to order first. TcpStream.connect already looped over candidates; the passive path and UdpSocket.send-to did not. Two user-visible failures follow. A UdpSocket bound to 127.0.0.1 and asked to send-to "localhost" resolved the destination without regard for its own family; where getaddrinfo returns ::1 first, sendto got an AF_INET6 sockaddr on an AF_INET socket and failed with EAFNOSUPPORT. And TcpListener.bind / UdpSocket.bind created a socket of the first candidate's family and gave up if socket() or bind() failed, even when a later candidate would have bound -- a host whose /etc/hosts lists "::1 localhost" while IPv6 is unavailable, or the ::1 address already occupied. bind_address owns socket creation, since the family is per-candidate, and iterates socket() -> setsockopt -> bind(), closing the fd and continuing on failure, failing only when every candidate does. Hints carry AI_PASSIVE. SO_REUSEADDR/SO_REUSEPORT stay TCP-only, as before. bind() errno is saved across close() so the message the caller reports is still the bind failure. resolve_address gains a prefer_family argument and keeps first-candidate behaviour when nothing matches, so a numeric host is unaffected. IPV6_V6ONLY is deliberately left at the host default: a specific-address bind is unaffected by it, so nothing in this change needs it, and pinning a value either way would silently alter existing wildcard "::" binds.
|
CI is green on both runners, and the job logs confirm neither new test took a skip path — no That settles the one caveat in the description: macOS did not report the address as rebindable, so the |
There was a problem hiding this comment.
Build & Tests
Checked out 5bb78f3 and ran all four suites the way CI does, reading the exit status from the bare command rather than through a pipe: tcp 7 / unix 6 / poll 6 / udp 6 passed, 0 failed, rc=0 on every one. No skipped: line printed anywhere, so nothing took a skip path here either.
CI run 31395570637 has head_sha 5bb78f3, so it tested this head. I read the per-step conclusions rather than the aggregate: Build test targets, Run tests, Install angler, Install carp-fmt, Lint, Format check and Generate docs are each success on ubuntu-latest and on macos-latest, and the workflow carries no continue-on-error, so the green is real per step. I also grepped both job logs for the skip strings rather than taking your follow-up comment at face value: none are present, and all three new tests print passed on both runners. The macOS EADDRINUSE precondition you flagged as unverified does hold there.
Merge-base with master is 512065a, master's tip, so the branch state is the merged state.
Both new tests are load-bearing — checked independently rather than taken from the description. Restoring master's src/ while keeping this branch's tests:
| suite | branch | master src/ + branch tests |
|---|---|---|
tcp_test |
7 passed, 0 failed, rc=0 |
6 passed, 1 failed, rc=1 — bind tries the next candidate when the first address is taken |
udp_test |
6 passed, 0 failed, rc=0 |
5 passed, 1 failed, rc=1 — send-to a dual-stack hostname reaches an IPv4-bound socket |
Exactly one failure each, and it is the new test in both. src/ restored clean afterwards.
Findings
One, and it is about what the passive fallback means rather than how it is written. The C itself I went through looking for the usual failure modes and did not find any: every candidate's fd is closed on the failure path, freeaddrinfo runs on both exits, bind()'s errno is saved across close(), SO_REUSEADDR/SO_REUSEPORT stay gated on SOCK_STREAM exactly as each path did before, getsockname still corrects addr after a port-0 bind on both the TCP and UDP paths, and prefer_family degrades to the old first-candidate behaviour when nothing matches — including the AF_UNSPEC that a zeroed bound would give. All three resolve_address call sites are updated.
Falling back after EADDRINUSE turns a loud failure into a silent misroute
With [::1]:P held by a foreign process — a plain listening socket with no SO_REUSEADDR/SO_REUSEPORT, i.e. nginx, or a Python one-liner, which is what I used:
master this branch
TcpListener.bind "localhost" P Error: Address Success, bound to 127.0.0.1:P
already in use
TcpStream.connect "localhost" P -- peer ::1 -> "FOREIGN-SERVER"
That last line is the problem. bind reports success, and a client on the same host asking for the same name is served by the other process, sending it whatever it would have sent yours. master refused to bind at all.
This is not an artefact of this box's resolver order. TcpListener_bind_ and TcpStream_connect_ walk the same candidate list, and I checked that AI_PASSIVE does not reorder it — getaddrinfo("localhost", …) returns ::1 127.0.0.1 with and without the flag here, as POSIX requires when nodename is non-NULL. connect stops at the first candidate that connects. So whenever the bind loop skips a candidate because something is already listening on it, that skipped candidate is by construction the one a same-host client reaches first, and something is listening there to answer. Reverse the resolver order and the same argument applies to whichever family comes first.
The two scenarios in the description are not the same shape, and only one of them has this problem.
- "a host whose
/etc/hostsstill lists::1 localhostwhile IPv6 is unavailable" —socket()orbind()fails withEAFNOSUPPORT/EADDRNOTAVAIL, nothing can ever be listening on that candidate, and continuing is unambiguously right. This is the case that motivates the change and it is pure win. - "With
::1:Poccupied by an unrelated socket" — the case theAddress already in usetranscript in the description is taken from — is the one above. For that scenario the fix replaces a diagnosable error with traffic going somewhere else.
So the question is whether the loop should continue on this address is taken at all, or only on this family is unusable. Narrowing it is small — save err, and stop the loop when it is EADDRINUSE — and it costs nothing that motivated the PR. It would, however, invert bind tries the next candidate when the first address is taken, which is built on exactly this case: the test occupies the first candidate with a real EADDRINUSE and asserts the fallback fires. So the decision decides the test, not the other way round.
Scope, honestly stated. The only non-test caller of TcpListener.bind in the org is Web.serve (web.carp:2885), and every documented example passes "0.0.0.0" — a single candidate, where none of this can arise. So this is a latent hazard in the API rather than a live bug in anything shipping today, and it needs a hostname, a foreign occupant, and a same-host client to bite. It is also what Go and Python do. I am raising it because it is silent, because the transcript in the description is the bad case, and because web.carp:2912 prints Listening on %s:%d with the requested host — so after a fallback the log says localhost while the socket is on 127.0.0.1 only.
UdpSocket.send-to is untouched by all of this and is the better half of the PR — preferring the socket's own family with a fall-back to first-candidate is strictly a superset of the old behaviour, and I could not construct a case where it is worse. The UDP bind path shares bind_address, so it inherits the same question, not a different one.
Verdict: revise
Everything mechanical is right — it builds, all four suites pass locally and on both CI runners per step, the two new tests demonstrably fail without the fix, and I found no defect in the C. What I want before this lands is a decision on one line: should the bind loop continue past a candidate that is merely occupied, given that a client asking for the same hostname then reaches the occupant? If you are happy with the trade-off, say so and the only change is a sentence in the two bind docstrings; if not, the EADDRINUSE guard is a couple of lines and the TCP test changes with it.
resolve_address(src/common.h) took the firstaddrinfogetaddrinforeturned and freed the rest, so all three call sites inherited whatever the resolver happened to order first.TcpStream.connectalready looped over candidates; the passive path andUdpSocket.send-todid not.Both consequences reproduce on a box where
getaddrinfo("localhost", …)returns::1before127.0.0.1(this one, per/etc/hosts).A —
UdpSocket.send-toignores the socket's own family.(UdpSocket.bind "127.0.0.1" 0)then(UdpSocket.send-to &s "localhost" p &data)resolved the destination to::1, sosendtogot anAF_INET6sockaddr on anAF_INETsocket:B — the passive path gives up after the first candidate. With
::1:Poccupied by an unrelated socket,TcpListener.bind "localhost" Pfailed outright even though127.0.0.1:Pwas free:The same shape applies to a host whose
/etc/hostsstill lists::1 localhostwhile IPv6 is unavailable.What changed
bind_addressincommon.howns socket creation — the family is per-candidate, so a pure address-out helper can't serve the bind path — and iteratessocket()→setsockopt→bind(), closing the fd and continuing on failure, failing only when every candidate does. Hints carryAI_PASSIVE.TcpListener_bind_andUdpSocket_bind_call it; thegetsockname()that reports the real port after binding to0is preserved, andlisten()stays outside the loop.resolve_addressgains aprefer_familyargument and keeps first-candidate behaviour when nothing matches.UdpSocket_send_MINUS_to_passesu->bound.ss_family.Details worth flagging for review:
SO_REUSEADDR/SO_REUSEPORTstay TCP-only, gated onsocktype == SOCK_STREAM, matching what each path did before.bind()'s errno is saved acrossclose(), so(System.error-text)still reports the bind failure rather than whatevercloseleft behind. Verified A/B againstmaster:Address already in use,Permission denied, andInvalid argumentfor a bogus and an empty host are byte-identical on both.socket/setsockopt/bindsequence once. The only literal difference issocket(…, rp->ai_protocol)instead ofsocket(…, 0), matching the existingTcpStream_connect_loop; forSOCK_STREAM/SOCK_DGRAMthese select the same protocol.IPV6_V6ONLYis deliberately left at the host default. Every candidate this loop can bind to is a specific address, andV6ONLYdoes not affect a specific-address bind — so nothing here needs it. Pinning a value either way would silently change existing wildcard"::"binds (Linux defaults to dual-stack, the BSDs to v6-only), which is outside this fix. Happy to set it if you'd rather it were explicit.Tests
Both new tests were confirmed to fail on unmodified
src/(tests applied,src/stashed →carp -xexits 1 on both suites) and pass on this branch, with no skip path taken.test/udp_test.carpround-trips a datagram sent tolocalhostfrom a socket bound explicitly to127.0.0.1, and again from one bound to::1. The pair is order-independent: whichever family the resolver puts first, the other half is the mismatched-family case. Each half is skipped, with a printed reason, iflocalhostlacks that family.test/tcp_test.carpoccupies whichever candidate the resolver returns first — viatest/addr_helpers.h, a plain listening socket with noSO_REUSEADDR/SO_REUSEPORT, on an OS-assigned port — then assertsTcpListener.bind "localhost" portstill succeeds. It skips with a printed reason whenlocalhostresolves to a single family. The helper also probes the occupied address with aSO_REUSEADDR|SO_REUSEPORTsocket and reports-2if the duplicate bind is permitted, so the test skips rather than passing vacuously on a platform with laxer reuse semantics (I can verify the EADDRINUSE precondition on Linux, not on macOS).All four suites built and run exactly as CI does (
carp -bthencarp -xontcp/unix/poll/udp, bare exit status): 7/6/6/6 passed, 0 failed.carp-fmt --checkandanglerclean over the same file set CI lints.docs/regenerated for the three docstring additions. NoCHANGELOG.mdin this repo, so none added.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.