imbajin commented on code in PR #3105:
URL: https://github.com/apache/hugegraph/pull/3105#discussion_r3663391671
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/util.sh:
##########
@@ -185,8 +344,17 @@ function wait_for_startup() {
return 1
fi
- status=$(curl -I -sS -k -w "%{http_code}" -o /dev/null "$server_url"
2> "$error_file_name")
- if [[ $status -eq 200 || $status -eq 401 ]]; then
+ # Bound each probe by the time left in the overall deadline: without
+ # --max-time a single blackholed request blocks past ${timeout_s}s.
+ # TODO(wait_for_startup): overshoot is now bounded but not zero - the
+ # loop still sleeps 2s after a probe and only then re-reads the clock,
+ # so the total can exceed ${timeout_s}s by roughly one sleep interval.
+ local remain_s=$((stop_s - now_s))
+ [ "$remain_s" -lt 1 ] && remain_s=1
+ local connect_s=$((remain_s < 5 ? remain_s : 5))
+ status=$(curl -I -sS -k --connect-timeout "$connect_s" --max-time
"$remain_s" \
Review Comment:
⚠️ Bounding each curl request does not make `wait_for_startup` honor its
overall timeout: after an immediate failed probe the loop always sleeps two
seconds before re-reading the clock. With `timeout_s=1`, an immediate curl
failure still returns after about two seconds, and the new test checks only the
curl flags rather than elapsed time. Please recompute the remaining deadline
before sleeping, cap the sleep to that remainder, and assert the end-to-end
duration.
##########
.github/workflows/server-ci.yml:
##########
@@ -70,17 +70,30 @@ jobs:
run: |
mvn clean compile -U -Dmaven.javadoc.skip=true -ntp
+ - name: Run check_port unit tests
+ if: ${{ env.BACKEND == 'rocksdb' }}
+ run: |
+ # Validates ss/netstat//dev/tcp replacement for lsof
Review Comment:
🧹 This comment still describes an `ss`/`netstat`/`/dev/tcp` fallback chain,
but the exact-head implementation contains no `/dev/tcp` branch and returns
`unknown` when neither socket-table tool is usable. Please update this and the
matching comments below to describe the actual `ss`/`netstat` behavior.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/util.sh:
##########
@@ -118,18 +119,175 @@ function process_id() {
return "$pid"
}
-# check the port of rest server is occupied
-function check_port() {
- local port=$(echo "$1" | sed 's|.*:||' | sed 's|/.*||')
- if ! command_available "lsof"; then
- echo "Required lsof but it is unavailable"
- exit 1
+# Extract a validated TCP port from a configured server URL.
+# Echoes the port on success. Returns 1 when the value carries no usable port
+# or is ambiguous, in which case the caller skips the preflight.
+function parse_port_from_url() {
+ local url="$1"
+
+ # ServerOptions tolerates surrounding whitespace, so strip it first.
+ url="${url#"${url%%[![:space:]]*}"}"
+ url="${url%"${url##*[![:space:]]}"}"
+ [[ -z "$url" ]] && return 1
+
+ # The scheme is optional and case-insensitive.
+ local scheme="" rest="$url"
+ if [[ "$url" == *"://"* ]]; then
+ scheme=$(echo "${url%%://*}" | tr '[:upper:]' '[:lower:]')
+ rest="${url#*://}"
fi
- lsof -i :"$port" >/dev/null
- if [ $? -eq 0 ]; then
- echo "The port $port has already been used"
- exit 1
+
+ # The authority ends at the first '/', '?' or '#'.
+ local authority="${rest%%[/?#]*}"
+ [[ -z "$authority" ]] && return 1
+
+ # Drop any userinfo prefix; its colon would otherwise look like an
+ # unbracketed IPv6 separator.
+ authority="${authority##*@}"
+ [[ -z "$authority" ]] && return 1
+
+ local port=""
+ if [[ "$authority" =~ ^\[[^]]*\](:([0-9]+))?$ ]]; then
+ # Bracketed IPv6, with or without a port: [::1] or [::1]:8080
+ port="${BASH_REMATCH[2]}"
+ elif [[ "$authority" == *:*:* ]]; then
+ # Unbracketed IPv6 is ambiguous: in "::1:8080" the trailing group may
+ # be a port or another hextet. Refuse to guess.
+ # TODO(check_port): no preflight runs at all for this form. If
+ # ServerOptions ever guarantees a normalized bracketed value here, this
+ # branch can resolve the port instead of skipping the check.
+ echo "WARN: ambiguous IPv6 authority '$authority' in server URL;" \
+ "use bracket notation such as [::1]:8080." >&2
+ return 1
+ elif [[ "$authority" == *:* ]]; then
+ port="${authority##*:}"
+ fi
+
+ # Fall back to the scheme's default port.
+ # TODO(check_port): a scheme-less value with no explicit port (e.g. plain
+ # "127.0.0.1") has no derivable port, so it is skipped rather than guessed.
+ # Reading the configured default from ServerOptions would close this gap.
+ if [[ -z "$port" ]]; then
+ case "$scheme" in
+ http) port="80" ;;
+ https) port="443" ;;
+ *) return 1 ;;
+ esac
+ fi
+
+ [[ "$port" =~ ^[0-9]+$ ]] || return 1
+ # Normalise leading-zero forms textually; Java reads 08080 as decimal 8080.
+ # Arithmetic conversion must not happen before the value is bounded: Bash
+ # evaluates in 64-bit and wraps silently, so 18446744073709559616 would
+ # otherwise pass the range check as port 8000.
+ port="${port#"${port%%[!0]*}"}"
+ [[ -z "$port" ]] && return 1
+ (( ${#port} <= 5 )) || return 1
+ (( port >= 1 && port <= 65535 )) || return 1
+
+ echo "$port"
+}
+
+# Echo "busy", "free" or "unknown" for the given TCP port.
+#
+# Detection is deliberately port-only, matching the conservative behaviour of
+# the `lsof -i :PORT` call this replaces. Reproducing kernel socket semantics
+# in Bash - dual-stack IPV6_V6ONLY, wildcard versus specific binds, address
+# canonicalisation - produced more wrong answers than it prevented, so we only
+# ask "is anything already listening on this port?".
+#
+# Only LISTEN rows and only the local-address column are inspected, so an
+# unrelated outbound connection to the same port number is never mistaken for
+# a local listener. A tool that is missing, fails, or yields no recognisable
+# listener row reports "unknown" rather than "free".
+#
+# TODO(check_port): port-only matching ignores the listener's address, so a
+# listener bound to one local address (127.0.0.1:8080) reports the port busy
+# even when the server would bind a different one (192.168.1.5:8080). This is
+# deliberate - it is what `lsof -i :PORT` did, and it fails safe - but it can
+# refuse a bind that would have succeeded. If that is reported in practice,
+# revisit by comparing the local-address column instead of only its port.
+#
+# TODO(check_port): a host with genuinely zero LISTEN sockets is indistinguish-
+# able from a restricted or unparseable table, so both report "unknown" and
+# warn on every start. Distinguishing them needs a positive signal that the
+# table was readable (e.g. an exit status ss/netstat do not currently give).
+function port_listen_state() {
+ local port="$1"
+ local out
+ local os
+ os=$(uname)
+
+ # $4 is the local address for both `ss -ltn` and BSD `netstat -an`.
+ # Splitting on the last separator keeps IPv6 hextets (for example
+ # [2001:db8::80]:443) from being misread as the port.
+ local parser='
+ NF >= 4 && (!want_listen || $NF == "LISTEN") {
+ addr = $4
+ cut = 0
+ for (k = length(addr); k > 0; k--) {
+ if (substr(addr, k, 1) == sep) { cut = k; break }
+ }
+ if (cut == 0) next
+ rows++
+ if (substr(addr, cut + 1) == port) { found = 1; exit }
+ }
+ END {
+ if (found) print "busy"
+ else if (rows > 0) print "free"
+ else print "unknown"
+ }'
+
+ if [[ "$os" == "Darwin" || "$os" == *BSD* ]]; then
+ if command_available "netstat" && out=$(netstat -an -p tcp
2>/dev/null) \
+ && [[ -n "$out" ]]; then
+ echo "$out" | awk -v port="$port" -v sep="." -v want_listen=1
"$parser"
+ return 0
+ fi
+ else
+ # `ss -H -ltn` already restricts output to listening sockets.
+ if command_available "ss" && out=$(ss -H -ltn 2>/dev/null) && [[ -n
"$out" ]]; then
+ echo "$out" | awk -v port="$port" -v sep=":" -v want_listen=0
"$parser"
+ return 0
Review Comment:
⚠️ A successful but unparseable `ss` response returns `unknown` here
immediately, so a working `netstat` fallback is never consulted. For example,
with `ss` emitting a non-empty diagnostic banner and `netstat` reporting
`0.0.0.0:8080 LISTEN`, `port_listen_state 8080` still returns `unknown` and
lets startup reach the later bind failure. Please capture the parser result and
return only for `busy` or `free`; continue to `netstat` on `unknown`, with a
regression covering this fallback.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/util.sh:
##########
@@ -118,18 +119,175 @@ function process_id() {
return "$pid"
}
-# check the port of rest server is occupied
-function check_port() {
- local port=$(echo "$1" | sed 's|.*:||' | sed 's|/.*||')
- if ! command_available "lsof"; then
- echo "Required lsof but it is unavailable"
- exit 1
+# Extract a validated TCP port from a configured server URL.
+# Echoes the port on success. Returns 1 when the value carries no usable port
+# or is ambiguous, in which case the caller skips the preflight.
+function parse_port_from_url() {
+ local url="$1"
+
+ # ServerOptions tolerates surrounding whitespace, so strip it first.
+ url="${url#"${url%%[![:space:]]*}"}"
+ url="${url%"${url##*[![:space:]]}"}"
+ [[ -z "$url" ]] && return 1
+
+ # The scheme is optional and case-insensitive.
+ local scheme="" rest="$url"
+ if [[ "$url" == *"://"* ]]; then
+ scheme=$(echo "${url%%://*}" | tr '[:upper:]' '[:lower:]')
+ rest="${url#*://}"
fi
- lsof -i :"$port" >/dev/null
- if [ $? -eq 0 ]; then
- echo "The port $port has already been used"
- exit 1
+
+ # The authority ends at the first '/', '?' or '#'.
+ local authority="${rest%%[/?#]*}"
+ [[ -z "$authority" ]] && return 1
+
+ # Drop any userinfo prefix; its colon would otherwise look like an
+ # unbracketed IPv6 separator.
+ authority="${authority##*@}"
+ [[ -z "$authority" ]] && return 1
+
+ local port=""
+ if [[ "$authority" =~ ^\[[^]]*\](:([0-9]+))?$ ]]; then
+ # Bracketed IPv6, with or without a port: [::1] or [::1]:8080
+ port="${BASH_REMATCH[2]}"
+ elif [[ "$authority" == *:*:* ]]; then
+ # Unbracketed IPv6 is ambiguous: in "::1:8080" the trailing group may
+ # be a port or another hextet. Refuse to guess.
+ # TODO(check_port): no preflight runs at all for this form. If
+ # ServerOptions ever guarantees a normalized bracketed value here, this
+ # branch can resolve the port instead of skipping the check.
+ echo "WARN: ambiguous IPv6 authority '$authority' in server URL;" \
+ "use bracket notation such as [::1]:8080." >&2
+ return 1
+ elif [[ "$authority" == *:* ]]; then
+ port="${authority##*:}"
+ fi
+
+ # Fall back to the scheme's default port.
+ # TODO(check_port): a scheme-less value with no explicit port (e.g. plain
+ # "127.0.0.1") has no derivable port, so it is skipped rather than guessed.
+ # Reading the configured default from ServerOptions would close this gap.
+ if [[ -z "$port" ]]; then
+ case "$scheme" in
+ http) port="80" ;;
+ https) port="443" ;;
+ *) return 1 ;;
+ esac
+ fi
+
+ [[ "$port" =~ ^[0-9]+$ ]] || return 1
+ # Normalise leading-zero forms textually; Java reads 08080 as decimal 8080.
+ # Arithmetic conversion must not happen before the value is bounded: Bash
+ # evaluates in 64-bit and wraps silently, so 18446744073709559616 would
+ # otherwise pass the range check as port 8000.
+ port="${port#"${port%%[!0]*}"}"
+ [[ -z "$port" ]] && return 1
+ (( ${#port} <= 5 )) || return 1
+ (( port >= 1 && port <= 65535 )) || return 1
+
+ echo "$port"
+}
+
+# Echo "busy", "free" or "unknown" for the given TCP port.
+#
+# Detection is deliberately port-only, matching the conservative behaviour of
+# the `lsof -i :PORT` call this replaces. Reproducing kernel socket semantics
+# in Bash - dual-stack IPV6_V6ONLY, wildcard versus specific binds, address
+# canonicalisation - produced more wrong answers than it prevented, so we only
+# ask "is anything already listening on this port?".
+#
+# Only LISTEN rows and only the local-address column are inspected, so an
+# unrelated outbound connection to the same port number is never mistaken for
+# a local listener. A tool that is missing, fails, or yields no recognisable
+# listener row reports "unknown" rather than "free".
+#
+# TODO(check_port): port-only matching ignores the listener's address, so a
+# listener bound to one local address (127.0.0.1:8080) reports the port busy
+# even when the server would bind a different one (192.168.1.5:8080). This is
+# deliberate - it is what `lsof -i :PORT` did, and it fails safe - but it can
+# refuse a bind that would have succeeded. If that is reported in practice,
+# revisit by comparing the local-address column instead of only its port.
+#
+# TODO(check_port): a host with genuinely zero LISTEN sockets is indistinguish-
+# able from a restricted or unparseable table, so both report "unknown" and
+# warn on every start. Distinguishing them needs a positive signal that the
+# table was readable (e.g. an exit status ss/netstat do not currently give).
+function port_listen_state() {
+ local port="$1"
+ local out
+ local os
+ os=$(uname)
+
+ # $4 is the local address for both `ss -ltn` and BSD `netstat -an`.
+ # Splitting on the last separator keeps IPv6 hextets (for example
+ # [2001:db8::80]:443) from being misread as the port.
+ local parser='
+ NF >= 4 && (!want_listen || $NF == "LISTEN") {
+ addr = $4
+ cut = 0
+ for (k = length(addr); k > 0; k--) {
+ if (substr(addr, k, 1) == sep) { cut = k; break }
+ }
+ if (cut == 0) next
+ rows++
+ if (substr(addr, cut + 1) == port) { found = 1; exit }
+ }
+ END {
+ if (found) print "busy"
+ else if (rows > 0) print "free"
+ else print "unknown"
+ }'
+
+ if [[ "$os" == "Darwin" || "$os" == *BSD* ]]; then
+ if command_available "netstat" && out=$(netstat -an -p tcp
2>/dev/null) \
+ && [[ -n "$out" ]]; then
+ echo "$out" | awk -v port="$port" -v sep="." -v want_listen=1
"$parser"
+ return 0
+ fi
+ else
+ # `ss -H -ltn` already restricts output to listening sockets.
+ if command_available "ss" && out=$(ss -H -ltn 2>/dev/null) && [[ -n
"$out" ]]; then
+ echo "$out" | awk -v port="$port" -v sep=":" -v want_listen=0
"$parser"
+ return 0
+ fi
+ if command_available "netstat" && out=$(netstat -ltn 2>/dev/null) \
+ && [[ -n "$out" ]]; then
+ echo "$out" | awk -v port="$port" -v sep=":" -v want_listen=1
"$parser"
+ return 0
+ fi
fi
+
+ # TODO(check_port): with neither ss nor netstat present (some minimal
Review Comment:
⚠️ This `unknown` path is now the normal path in the official server images:
both `hugegraph-server/Dockerfile` variants install `lsof`, but neither
installs `ss` (`iproute2`) nor `netstat` (`net-tools`). The image therefore
loses the occupied-port preflight even though it still ships the old probe
dependency; a duplicate start can overwrite `bin/pid` before the new JVM fails
to bind, leaving the original process unmanaged by the stop script. Please
retain an `lsof` fallback or update the image dependency to a supported
socket-table tool, and add an image-level duplicate-start/stop test.
--
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]
