github-actions[bot] commented on code in PR #41035:
URL: https://github.com/apache/beam/pull/41035#discussion_r4495095921
##########
sdks/python/scripts/run_expansion_services.sh:
##########
@@ -108,31 +108,69 @@ if [[ $CHECK_FLOCK -eq 0 ]]; then
fi
fi
+wait_for_port() {
+ local port=$1
+ local srv_pid=$2
+ local name=$3
+ local logfile=$4
+ local elapsed=0
+ while [ $elapsed -lt 60 ]; do
+ if ! kill -0 "$srv_pid" >/dev/null 2>&1; then
+ echo "$name expansion service (pid $srv_pid) exited prematurely. Logs:"
+ cat "$logfile" || true
+ return 1
+ fi
+ if python -c "import socket, sys; s = socket.socket(); res =
s.connect_ex(('localhost', int(sys.argv[1]))); s.close(); sys.exit(0 if res ==
0 else 1)" "$port" >/dev/null 2>&1; then
+ echo "$name expansion service is listening @ $port"
+ return 0
+ fi
+ sleep 1
+ elapsed=$((elapsed + 1))
+ done
+ echo "Timed out waiting 60s for $name expansion service @ $port. Logs:"
+ cat "$logfile" || true
+ return 1
+}
+
case $STARTSTOP in
start)
if [ -f "$pid" ]; then
echo "services already running."
exit 0
fi
+ source $PYTHON_VIRTUALENV_DIR/bin/activate
+
echo "Launching Java expansion service @ $JAVA_PORT"
- java -jar $JAVA_EXPANSION_SERVICE_JAR $JAVA_PORT
--javaClassLookupAllowlistFile="$JAVA_EXPANSION_SERVICE_ALLOWLIST_FILE"
>$TEMP_DIR/$FILE_BASE-java.log 2>&1 </dev/null &
- mypid=$!
- if kill -0 $mypid >/dev/null 2>&1; then
- echo $mypid >> $pid
+ if [[ $CHECK_FLOCK -eq 0 ]]; then
+ java -jar $JAVA_EXPANSION_SERVICE_JAR $JAVA_PORT
--javaClassLookupAllowlistFile="$JAVA_EXPANSION_SERVICE_ALLOWLIST_FILE"
>$TEMP_DIR/$FILE_BASE-java.log 2>&1 </dev/null 200>&- &
+ else
+ java -jar $JAVA_EXPANSION_SERVICE_JAR $JAVA_PORT
--javaClassLookupAllowlistFile="$JAVA_EXPANSION_SERVICE_ALLOWLIST_FILE"
>$TEMP_DIR/$FILE_BASE-java.log 2>&1 </dev/null &
+ fi
+ java_pid=$!
+ if kill -0 $java_pid >/dev/null 2>&1; then
+ echo $java_pid >> $pid
else
echo "Can't start Java expansion service."
+ exit 1
fi
echo "Launching Python expansion service @ $PYTHON_PORT"
- source $PYTHON_VIRTUALENV_DIR/bin/activate
- python -m $PYTHON_EXPANSION_SERVICE_MODULE -p $PYTHON_PORT
--fully_qualified_name_glob "$PYTHON_EXPANSION_SERVICE_ALLOWLIST_GLOB"
>$TEMP_DIR/$FILE_BASE-python.log 2>&1 </dev/null &
- mypid=$!
- if kill -0 $mypid >/dev/null 2>&1; then
- echo $mypid >> $pid
+ if [[ $CHECK_FLOCK -eq 0 ]]; then
+ python -m $PYTHON_EXPANSION_SERVICE_MODULE -p $PYTHON_PORT
--fully_qualified_name_glob "$PYTHON_EXPANSION_SERVICE_ALLOWLIST_GLOB"
>$TEMP_DIR/$FILE_BASE-python.log 2>&1 </dev/null 200>&- &
+ else
+ python -m $PYTHON_EXPANSION_SERVICE_MODULE -p $PYTHON_PORT
--fully_qualified_name_glob "$PYTHON_EXPANSION_SERVICE_ALLOWLIST_GLOB"
>$TEMP_DIR/$FILE_BASE-python.log 2>&1 </dev/null &
+ fi
+ python_pid=$!
+ if kill -0 $python_pid >/dev/null 2>&1; then
+ echo $python_pid >> $pid
else
echo "Can't start Python expansion service."
+ exit 1
fi
+
+ wait_for_port "$JAVA_PORT" "$java_pid" "Java"
"$TEMP_DIR/$FILE_BASE-java.log"
Review Comment:
**🟠High**: Because this script does not enable `set -e`, a failure (return
code `1`) from `wait_for_port` here is ignored: execution continues to the
Python `wait_for_port` check and then to `flock -u 200` on line 191, which
resets `$?` to `0` and causes the script to exit successfully even if the Java
expansion service exited prematurely or timed out. Append `|| exit 1` so the
script fails fast (the kernel will automatically release the `flock` on FD 200
upon process exit now that child processes close FD 200).
```suggestion
wait_for_port "$JAVA_PORT" "$java_pid" "Java"
"$TEMP_DIR/$FILE_BASE-java.log" || exit 1
```
##########
sdks/python/scripts/run_expansion_services.sh:
##########
@@ -108,31 +108,69 @@ if [[ $CHECK_FLOCK -eq 0 ]]; then
fi
fi
+wait_for_port() {
+ local port=$1
+ local srv_pid=$2
+ local name=$3
+ local logfile=$4
+ local elapsed=0
+ while [ $elapsed -lt 60 ]; do
+ if ! kill -0 "$srv_pid" >/dev/null 2>&1; then
+ echo "$name expansion service (pid $srv_pid) exited prematurely. Logs:"
+ cat "$logfile" || true
+ return 1
+ fi
+ if python -c "import socket, sys; s = socket.socket(); res =
s.connect_ex(('localhost', int(sys.argv[1]))); s.close(); sys.exit(0 if res ==
0 else 1)" "$port" >/dev/null 2>&1; then
+ echo "$name expansion service is listening @ $port"
+ return 0
+ fi
+ sleep 1
+ elapsed=$((elapsed + 1))
+ done
+ echo "Timed out waiting 60s for $name expansion service @ $port. Logs:"
+ cat "$logfile" || true
+ return 1
+}
+
case $STARTSTOP in
start)
if [ -f "$pid" ]; then
echo "services already running."
exit 0
fi
+ source $PYTHON_VIRTUALENV_DIR/bin/activate
+
echo "Launching Java expansion service @ $JAVA_PORT"
- java -jar $JAVA_EXPANSION_SERVICE_JAR $JAVA_PORT
--javaClassLookupAllowlistFile="$JAVA_EXPANSION_SERVICE_ALLOWLIST_FILE"
>$TEMP_DIR/$FILE_BASE-java.log 2>&1 </dev/null &
- mypid=$!
- if kill -0 $mypid >/dev/null 2>&1; then
- echo $mypid >> $pid
+ if [[ $CHECK_FLOCK -eq 0 ]]; then
+ java -jar $JAVA_EXPANSION_SERVICE_JAR $JAVA_PORT
--javaClassLookupAllowlistFile="$JAVA_EXPANSION_SERVICE_ALLOWLIST_FILE"
>$TEMP_DIR/$FILE_BASE-java.log 2>&1 </dev/null 200>&- &
+ else
+ java -jar $JAVA_EXPANSION_SERVICE_JAR $JAVA_PORT
--javaClassLookupAllowlistFile="$JAVA_EXPANSION_SERVICE_ALLOWLIST_FILE"
>$TEMP_DIR/$FILE_BASE-java.log 2>&1 </dev/null &
+ fi
+ java_pid=$!
+ if kill -0 $java_pid >/dev/null 2>&1; then
+ echo $java_pid >> $pid
else
echo "Can't start Java expansion service."
+ exit 1
fi
echo "Launching Python expansion service @ $PYTHON_PORT"
- source $PYTHON_VIRTUALENV_DIR/bin/activate
- python -m $PYTHON_EXPANSION_SERVICE_MODULE -p $PYTHON_PORT
--fully_qualified_name_glob "$PYTHON_EXPANSION_SERVICE_ALLOWLIST_GLOB"
>$TEMP_DIR/$FILE_BASE-python.log 2>&1 </dev/null &
- mypid=$!
- if kill -0 $mypid >/dev/null 2>&1; then
- echo $mypid >> $pid
+ if [[ $CHECK_FLOCK -eq 0 ]]; then
+ python -m $PYTHON_EXPANSION_SERVICE_MODULE -p $PYTHON_PORT
--fully_qualified_name_glob "$PYTHON_EXPANSION_SERVICE_ALLOWLIST_GLOB"
>$TEMP_DIR/$FILE_BASE-python.log 2>&1 </dev/null 200>&- &
+ else
+ python -m $PYTHON_EXPANSION_SERVICE_MODULE -p $PYTHON_PORT
--fully_qualified_name_glob "$PYTHON_EXPANSION_SERVICE_ALLOWLIST_GLOB"
>$TEMP_DIR/$FILE_BASE-python.log 2>&1 </dev/null &
+ fi
+ python_pid=$!
+ if kill -0 $python_pid >/dev/null 2>&1; then
+ echo $python_pid >> $pid
else
echo "Can't start Python expansion service."
+ exit 1
fi
+
+ wait_for_port "$JAVA_PORT" "$java_pid" "Java"
"$TEMP_DIR/$FILE_BASE-java.log"
+ wait_for_port "$PYTHON_PORT" "$python_pid" "Python"
"$TEMP_DIR/$FILE_BASE-python.log"
Review Comment:
**🟠High**: When `CHECK_FLOCK -eq 0`, the subsequent `flock -u 200` on line
191 overwrites the non-zero return status of this `wait_for_port` call, causing
`run_expansion_services.sh` to exit with status `0` even when the Python
expansion service crashes or times out. Append `|| exit 1` to propagate the
failure to the caller.
```suggestion
wait_for_port "$PYTHON_PORT" "$python_pid" "Python"
"$TEMP_DIR/$FILE_BASE-python.log" || exit 1
```
--
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]
