bitflicker64 commented on PR #3105: URL: https://github.com/apache/hugegraph/pull/3105#issuecomment-5092882079
Thanks for the detailed design in the last review — I took it in full rather than patching around the edges. Current head: `ea81ad76`. ### What changed `check_port` is rewritten to the minimal design. It now answers one question — *is anything already listening on this port?* — and reports `busy`, `free` or `unknown`, with the bind left authoritative. - Parse: case-insensitive scheme, explicit/default port, scheme-less value, bracketed IPv6. Ambiguous unbracketed IPv6 is rejected with a warning instead of guessed. - Detect: `ss -H -ltn` on Linux (falling back to `netstat -ltn`), `netstat -an -p tcp` on BSD — only `LISTEN` rows, only the local-address column, address split on its **last** separator. - Removed: DNS resolution, address canonicalization, cross-family matching, `/dev/tcp` and its watchdog. `normalize_addr()` and `run_with_deadline()` are gone entirely. Worth noting this *strengthens* the original goal rather than only preserving it: there is no longer any unbounded call left in the startup path, not merely no `lsof`. All 12 items from your list are addressed — the resolution table is in the PR description. Items 2, 3, 6 and 8 are resolved by removal rather than by more shell. ### One deviation You suggested `fuser` as the Linux fallback; I used `netstat` instead. `fuser` cannot distinguish "not found" from "insufficient privileges", so it would collapse `free` and `unknown` into one result and break the tri-state contract. Happy to switch if you would rather have `fuser` there. ### Size | | Before | Now | |---|---|---| | PR vs `master` | +1731 / −189 | +859 / −188 | | server `util.sh` | 849 | 617 | | `test-check-port.sh` | 984 | 320 | The convergence commit itself is +426 / −1297. ### What this gives up Port-only detection trades away real capability, so rather than leave that implicit I marked each gap in place with a namespaced `TODO`: - Port-only matching ignores the listener's address, so a listener on `127.0.0.1:8080` reports the port busy even when the server would bind `192.168.1.5:8080`. That is the old `lsof -i :PORT` behaviour and fails safe, but it can refuse a bind that would have succeeded — it is the one limitation here that can block a legitimate start. - Zero `LISTEN` sockets is indistinguishable from a restricted table; both report `unknown` and warn. - With neither `ss` nor `netstat` present there is no probe left. A dependency-free fallback would need a bounded connect, which is exactly what was removed — so that one is not a simple follow-up. `grep -rn "TODO(check_port)"` finds them; the full table is in the description. ### Tests `test-check-port.sh` is now a compact contract suite (984 → 320 lines): URL-to-port, `ss` busy/free/failure, BSD `LISTEN` vs `ESTABLISHED` vs `TIME_WAIT`, the `unknown` fallback, one real ephemeral listener that asserts the child actually bound, rename failure, and a bounded startup probe. **44 passed, 0 failed.** I checked it is not vacuous by mutating the implementation — dropping the `LISTEN` filter, downgrading `unknown` to `free`, and reading the foreign-address column each fail the suite. The first mutation initially passed, which is how the `TIME_WAIT` case got added. Your macOS repro, before and after on this head: ``` 37 ESTABLISHED to :443, no listener on 443 before: check_port "http://0.0.0.0:443"; -> "The port 443 has already been used", exit 1 after : -> exit 0 (free) ``` Auditing those gaps also turned up a defect worth mentioning: a URL with userinfo (`http://user:pass@host:port`) was caught by the unbracketed-IPv6 check, so it skipped the preflight and printed a misleading warning about bracket notation. Fixed and covered. ### Out of scope Two findings are deliberately not addressed, with reasons on their threads: the `libjemalloc_aarch64.so` MD5 (pre-existing `master` value; this PR only added quoting) and the mirror-only `cluster-test` failure (identical signature on branches that do not touch this code). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
