imbajin commented on code in PR #3258:
URL: https://github.com/apache/hugegraph/pull/3258#discussion_r4238494468
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/hugegraph-server.sh:
##########
@@ -252,13 +261,90 @@ if [ "${OPEN_TELEMETRY}" == "true" ]; then
export OTEL_RESOURCE_ATTRIBUTES=service.name=server
fi
-if [[ "${STDOUT_MODE:-false}" != "true" ]]; then
- # Daemon stderr only reaches hugegraph-server-stdout.log, so a bootstrap
- # rejection of the effective DNS policy (a broken operator override
- # included) would be invisible in the log start-hugegraph.sh points
- # operators at. Let the bootstrap mirror its fatal errors there.
- JVM_OPTIONS="${JVM_OPTIONS} -Dhugegraph.bootstrap.error.log=${OUTPUT}"
-fi
+# Let the bootstrap mirror its fatal errors (a rejected DNS policy, a broken
+# operator override included) into hugegraph-server.log. In daemon mode its
+# stderr only reaches hugegraph-server-stdout.log; in STDOUT_MODE it reaches
the
+# container log, which keeps a single earlier generation, so the file copy is
+# what survives a crash loop on a mounted logs volume.
+JVM_OPTIONS="${JVM_OPTIONS} -Dhugegraph.bootstrap.error.log=${OUTPUT}"
+
+# Keep heap dumps and JVM crash logs in $LOGS whatever JAVA_OPTIONS holds. The
+# defaults come first in JAVA_TOOL_OPTIONS, ahead of the operator's own value.
The
+# JVM applies JAVA_TOOL_OPTIONS before JDK_JAVA_OPTIONS, the command line
+# (JAVA_OPTIONS and -j) and _JAVA_OPTIONS, and the last occurrence of a flag
wins,
+# so any of those overrides a default. The JVM does the parsing itself,
including
+# quoted options, and @argfiles in JDK_JAVA_OPTIONS or on the command line.
+# Child JVMs the Server starts (computer jobs, for example) inherit the
+# environment but not the command line, so they get these defaults too, and
every
+# path must stay unique per JVM. ErrorFile expands %p to each JVM's PID.
+# HeapDumpPath expands %p only from JDK 25, but on every version, when it
names an
+# existing directory, each JVM writes java_pid<its pid>.hprof inside it, so it
+# points at one directory per launch. HotSpot picks that name, so a child JVM
that
+# gets a PID reused within the same launch cannot write over an earlier child's
+# dump; the launcher cannot rename HotSpot's dump file.
+# A restarted container often reuses the PID; HotSpot truncates an existing
crash
+# log (JDK 17+) and will not write a heap dump over an existing file. So the
names
+# carry the host name (the pod name on Kubernetes, so pods sharing one log
volume
+# do not collide), the launch time, and a counter. A plain mkdir claims each
name
+# atomically, so concurrent launches never share one. This runs after every
+# preflight check, so a launch that fails before Java starts leaves nothing.
+# Dump directories are never removed automatically: one stays empty until a JVM
+# using it runs out of memory, and that JVM may be a computer job that outlives
+# the Server, so the launcher cannot tell when a directory is safe to delete.
+crash_name_taken() {
+ local name="$1" file restore_failglob taken=1
+ [[ -e "${LOGS}/heapdump_${name}" || -L "${LOGS}/heapdump_${name}" ]] &&
return 0
+ restore_failglob=$(shopt -p failglob)
+ shopt -u failglob
+ for file in "${LOGS}"/hs_err_pid*_"${name}".log; do
+ if [[ -e ${file} || -L ${file} ]]; then
+ taken=0
+ break
+ fi
+ done
+ eval "${restore_failglob}"
+ return ${taken}
+}
+case "${LOGS}" in
+ *\"*|*%*)
+ # The defaults below cannot carry this path: they quote it with double
quotes
+ # inside JAVA_TOOL_OPTIONS, and ErrorFile expands %. Keep the base
behaviour
+ # instead: heap dumps into $LOGS on the Server's command line, ahead of
+ # JAVA_OPTIONS so an operator value there still wins, and no ErrorFile
default.
+ report_error "WARN: ${LOGS} contains a double quote or %, so heap
dumps go to ${LOGS}\
+ without a per-launch directory and no crash log default is set; set
-XX:ErrorFile yourself"
+ JAVA_OPTIONS="-XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=${LOGS}
${JAVA_OPTIONS}"
Review Comment:
⚠️ This branch puts `-XX:+HeapDumpOnOutOfMemoryError` into `JAVA_OPTIONS`,
which the JVM processes after `JAVA_TOOL_OPTIONS`. With `LOGS` containing `%`
or a double quote, setting the documented
`JAVA_TOOL_OPTIONS=-XX:-HeapDumpOnOutOfMemoryError` opt-out is therefore
overridden for the Server process, despite the README saying it disables dumps
for every JVM. Please preserve that opt-out in this fallback or document the
fallback-specific setting. Evidence: this assignment is expanded on the Java
command line below, after the environment options.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/hugegraph-server.sh:
##########
@@ -252,13 +261,90 @@ if [ "${OPEN_TELEMETRY}" == "true" ]; then
export OTEL_RESOURCE_ATTRIBUTES=service.name=server
fi
-if [[ "${STDOUT_MODE:-false}" != "true" ]]; then
- # Daemon stderr only reaches hugegraph-server-stdout.log, so a bootstrap
- # rejection of the effective DNS policy (a broken operator override
- # included) would be invisible in the log start-hugegraph.sh points
- # operators at. Let the bootstrap mirror its fatal errors there.
- JVM_OPTIONS="${JVM_OPTIONS} -Dhugegraph.bootstrap.error.log=${OUTPUT}"
-fi
+# Let the bootstrap mirror its fatal errors (a rejected DNS policy, a broken
+# operator override included) into hugegraph-server.log. In daemon mode its
+# stderr only reaches hugegraph-server-stdout.log; in STDOUT_MODE it reaches
the
+# container log, which keeps a single earlier generation, so the file copy is
+# what survives a crash loop on a mounted logs volume.
+JVM_OPTIONS="${JVM_OPTIONS} -Dhugegraph.bootstrap.error.log=${OUTPUT}"
+
+# Keep heap dumps and JVM crash logs in $LOGS whatever JAVA_OPTIONS holds. The
+# defaults come first in JAVA_TOOL_OPTIONS, ahead of the operator's own value.
The
+# JVM applies JAVA_TOOL_OPTIONS before JDK_JAVA_OPTIONS, the command line
+# (JAVA_OPTIONS and -j) and _JAVA_OPTIONS, and the last occurrence of a flag
wins,
+# so any of those overrides a default. The JVM does the parsing itself,
including
+# quoted options, and @argfiles in JDK_JAVA_OPTIONS or on the command line.
+# Child JVMs the Server starts (computer jobs, for example) inherit the
+# environment but not the command line, so they get these defaults too, and
every
+# path must stay unique per JVM. ErrorFile expands %p to each JVM's PID.
+# HeapDumpPath expands %p only from JDK 25, but on every version, when it
names an
+# existing directory, each JVM writes java_pid<its pid>.hprof inside it, so it
+# points at one directory per launch. HotSpot picks that name, so a child JVM
that
+# gets a PID reused within the same launch cannot write over an earlier child's
+# dump; the launcher cannot rename HotSpot's dump file.
+# A restarted container often reuses the PID; HotSpot truncates an existing
crash
+# log (JDK 17+) and will not write a heap dump over an existing file. So the
names
+# carry the host name (the pod name on Kubernetes, so pods sharing one log
volume
+# do not collide), the launch time, and a counter. A plain mkdir claims each
name
+# atomically, so concurrent launches never share one. This runs after every
+# preflight check, so a launch that fails before Java starts leaves nothing.
+# Dump directories are never removed automatically: one stays empty until a JVM
+# using it runs out of memory, and that JVM may be a computer job that outlives
+# the Server, so the launcher cannot tell when a directory is safe to delete.
+crash_name_taken() {
+ local name="$1" file restore_failglob taken=1
+ [[ -e "${LOGS}/heapdump_${name}" || -L "${LOGS}/heapdump_${name}" ]] &&
return 0
+ restore_failglob=$(shopt -p failglob)
+ shopt -u failglob
+ for file in "${LOGS}"/hs_err_pid*_"${name}".log; do
+ if [[ -e ${file} || -L ${file} ]]; then
+ taken=0
+ break
+ fi
+ done
+ eval "${restore_failglob}"
+ return ${taken}
+}
+case "${LOGS}" in
+ *\"*|*%*)
+ # The defaults below cannot carry this path: they quote it with double
quotes
+ # inside JAVA_TOOL_OPTIONS, and ErrorFile expands %. Keep the base
behaviour
+ # instead: heap dumps into $LOGS on the Server's command line, ahead of
+ # JAVA_OPTIONS so an operator value there still wins, and no ErrorFile
default.
+ report_error "WARN: ${LOGS} contains a double quote or %, so heap
dumps go to ${LOGS}\
+ without a per-launch directory and no crash log default is set; set
-XX:ErrorFile yourself"
+ JAVA_OPTIONS="-XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=${LOGS}
${JAVA_OPTIONS}"
+ ;;
+ *)
+ LAUNCH_HOST=$(printf '%s' "${HOSTNAME:-localhost}" | LC_ALL=C tr -c
'A-Za-z0-9._-' '_')
+ LAUNCH_HOST="${LAUNCH_HOST:0:64}"
+ LAUNCH_STAMP="${LAUNCH_HOST}_$(date +%Y%m%d-%H%M%S)"
+ LAUNCH_ID="${LAUNCH_STAMP}"
+ LAUNCH_SUFFIX=0
+ HEAP_DUMP_PATH=""
+ while [[ -z ${HEAP_DUMP_PATH} ]]; do
+ if ! crash_name_taken "${LAUNCH_ID}" &&
+ mkdir "${LOGS}/heapdump_${LAUNCH_ID}" 2>/dev/null; then
+ HEAP_DUMP_PATH="${LOGS}/heapdump_${LAUNCH_ID}"
+ elif [[ ${LAUNCH_SUFFIX} -ge 1000 ]]; then
+ report_error "WARN: heap dump names for ${LAUNCH_STAMP} are
all taken; heap dumps go to ${LOGS}"
+ HEAP_DUMP_PATH="${LOGS}"
+ elif ! crash_name_taken "${LAUNCH_ID}"; then
+ # Not a name clash (a full disk, for example): keep starting,
and dump
+ # into $LOGS itself rather than make diagnostics a startup
requirement.
+ report_error "WARN: cannot create
${LOGS}/heapdump_${LAUNCH_ID}; heap dumps go to ${LOGS}"
+ HEAP_DUMP_PATH="${LOGS}"
+ else
+ LAUNCH_SUFFIX=$((LAUNCH_SUFFIX + 1))
+ LAUNCH_ID="${LAUNCH_STAMP}-${LAUNCH_SUFFIX}"
+ fi
+ done
+ CRASH_OPTIONS="-XX:+HeapDumpOnOutOfMemoryError"
+ CRASH_OPTIONS="${CRASH_OPTIONS} \"-XX:HeapDumpPath=${HEAP_DUMP_PATH}\""
+ CRASH_OPTIONS="${CRASH_OPTIONS}
\"-XX:ErrorFile=${LOGS}/hs_err_pid%p_${LAUNCH_ID}.log\""
Review Comment:
⚠️ All JVMs launched during this Server run inherit this `JAVA_TOOL_OPTIONS`
value, and the only per-process part of this path is `%p`. If a computer-job
JVM writes an `hs_err` file and a later child reuses its PID before the same
Server exits, both resolve to this exact path; the launcher notes that HotSpot
truncates existing crash logs on JDK 17+, so the later crash erases the earlier
report. Please make child `ErrorFile` names unique or document this overwrite
case. Evidence: `AbstractComputer` uses a default `ProcessBuilder` without
clearing the inherited environment, and this option is set here.
--
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]
