Issue #10473 - Revert jetty.sh pgrep, and update started checks (#10501)

* Revert pgrep usage in jetty.sh
* Adding test for jetty.conf
* Correcting renamed xml file
* Improved started check
+ Improved `started` function code
  - adding comment explaining steps
  - adding named parameters
+ Improved ARGS check for "jetty.state="
  option, to know when to check the
  state file.
* Make sure state.mod is before any deploy steps to ensure jetty-state file is created early.
* Early cleanup / creation of State file
* Improved `started` function
+ Don't attempt to read from State File
  if it doesn't exist
+ Don't attempt to read from PID File
  if it doesn't exist
+ DEBUG in state file logic
+ DEBUG in pid file logic
+ proper startsWith logic for state detection
* Better state debugging and pid kill

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

jetty-home/src/main/resources/bin/jetty.sh

@@ -118,35 +118,92 @@ findDirectory()
   done
 }
 
+# test if process specified in PID file is still running
 running()
 {
-  if [ -f "$1" ]
+  local PIDFILE=$1
-  then
+  if [ -r "$PIDFILE" ] ; then
-    local PID=$(cat "$1" 2>/dev/null) || return 1
+    local PID=$(tail -1 "$PIDFILE")
-    kill -0 "$PID" 2>/dev/null
+    if kill -0 "$PID" 2>/dev/null ; then
-    return
+      return 0
+    fi
   fi
-  rm -f "$1"
   return 1
 }
 
+# Test state file (after timeout) for started state
 started()
 {
-  # wait for 60s to see "STARTED" in PID file, needs jetty-started.xml as argument
+  local STATEFILE=$1
-  for ((T = 0; T < $(($3 / 4)); T++))
+  local PIDFILE=$2
+  local STARTTIMEOUT=$3
+  # wait till timeout to see "STARTED" in state file, needs --module=state as argument
+  for ((T = 0; T < $STARTTIMEOUT; T++))
   do
-    sleep 4
+    echo -n "."
-    [ -z "$(tail -1 $1 | grep STARTED 2>/dev/null)" ] || return 0
+    sleep 1
-    [ -z "$(tail -1 $1 | grep STOPPED 2>/dev/null)" ] || return 1
+    if [ -r $STATEFILE ] ; then
-    [ -z "$(tail -1 $1 | grep FAILED 2>/dev/null)" ] || return 1
+      STATENOW=$(tail -1 $STATEFILE)
-    local PID=$(cat "$2" 2>/dev/null) || return 1
+      (( DEBUG )) && echo "State (now): $STATENOW"
-    kill -0 "$PID" 2>/dev/null || return 1
+      case "$STATENOW" in
-    echo -n ". "
+        STARTED*)
+          echo " started"
+          return 0;;
+        STOPPED*)
+          echo " stopped"
+          return 1;;
+        FAILED*)
+          echo " failed"
+          return 1;;
+      esac
+    else
+      (( DEBUG )) && echo "Unable to read State File: $STATEFILE"
+    fi
   done
-
+  (( DEBUG )) && echo "Timeout $STARTTIMEOUT expired waiting for start state from $STATEFILE"
+  echo " timeout"
   return 1;
 }
 
+pidKill()
+{
+  local PIDFILE=$1
+  local TIMEOUT=$2
+
+  if [ -r $PIDFILE ] ; then
+    local PID=$(tail -1 "$PIDFILE")
+    if [ -z "$PID" ] ; then
+      echo "ERROR: no pid found in $PIDFILE"
+      return 1
+    fi
+
+    # Try default kill first
+    if kill -0 "$PID" 2>/dev/null ; then
+      (( DEBUG )) && echo "PID=$PID is running, sending kill"
+      kill "$PID" 2>/dev/null
+    else
+      rm -f $PIDFILE 2> /dev/null
+      return 0
+    fi
+
+    # Perform harsh kill next
+    while kill -0 "$PID" 2>/dev/null
+    do
+      if (( TIMEOUT-- == 0 )) ; then
+        (( DEBUG )) && echo "PID=$PID is running, sending kill signal=KILL (TIMEOUT=$TIMEOUT)"
+        kill -KILL "$PID" 2>/dev/null
+      fi
+      echo -n "."
+      sleep 1
+    done
+    echo "Killed $PID"
+    return 0
+  else
+    (( DEBUG )) && echo "Unable to read PID File: $PIDFILE"
+    return 1
+  fi
+}
+
 
 readConfig()
 {
@@ -436,10 +493,6 @@ esac
 JETTY_DRY_RUN=$("$JAVA" -jar "$JETTY_START" --dry-run=opts,path,main,args ${JETTY_ARGS[*]} ${JAVA_OPTIONS[*]})
 RUN_ARGS=($JETTY_SYS_PROPS ${JETTY_DRY_RUN[@]})
 
-#####################################################
-# Comment these out after you're happy with what
-# the script is doing.
-#####################################################
 if (( DEBUG ))
 then
   dumpEnv
@@ -497,22 +550,23 @@ case "$ACTION" in
         su - "$JETTY_USER" $SU_SHELL -c "
           cd \"$JETTY_BASE\"
           echo ${RUN_ARGS[*]} start-log-file=\"$JETTY_START_LOG\" | xargs ${JAVA} > /dev/null &
-          disown $(pgrep -P $!)"
+          disown \$!"
       else
         # Startup if not switching users
         echo ${RUN_ARGS[*]} | xargs ${JAVA} > /dev/null &
-        disown $(pgrep -P $!)
+        disown $!
       fi
 
     fi
 
-    if expr "${JETTY_ARGS[*]}" : '.*jetty-started.xml.*' >/dev/null
+    if expr "${JETTY_ARGS[*]}" : '.*jetty\.state=.*' >/dev/null
     then
       if started "$JETTY_STATE" "$JETTY_PID" "$JETTY_START_TIMEOUT"
       then
         echo "OK `date`"
       else
         echo "FAILED `date`"
+        pidKill $JETTY_PID 30
         exit 1
       fi
     else
@@ -537,26 +591,12 @@ case "$ACTION" in
       done
     else
       # Stop from a non-service path
-      if [ ! -f "$JETTY_PID" ] ; then
+      if [ ! -r "$JETTY_PID" ] ; then
         echo "ERROR: no pid found at $JETTY_PID"
         exit 1
       fi
 
-      PID=$(cat "$JETTY_PID" 2>/dev/null)
+      pidKill "$JETTY_PID" 30
-      if [ -z "$PID" ] ; then
-        echo "ERROR: no pid id found in $JETTY_PID"
-        exit 1
-      fi
-      kill "$PID" 2>/dev/null
-
-      TIMEOUT=30
-      while running $JETTY_PID; do
-        if (( TIMEOUT-- == 0 )); then
-          kill -KILL "$PID" 2>/dev/null
-        fi
-
-        sleep 1
-      done
     fi
 
     rm -f "$JETTY_PID"

jetty-server/src/main/config/modules/state.mod

@@ -9,6 +9,9 @@ start
 [depends]
 server
 
+[before]
+deploy
+
 [xml]
 etc/jetty-state.xml
 

jetty-server/src/main/java/org/eclipse/jetty/server/StateLifeCycleListener.java

@@ -13,6 +13,7 @@
 
 package org.eclipse.jetty.server;
 
+import java.io.IOException;
 import java.io.Writer;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -24,7 +25,7 @@ import org.slf4j.LoggerFactory;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.nio.file.StandardOpenOption.APPEND;
-import static java.nio.file.StandardOpenOption.CREATE;
+import static java.nio.file.StandardOpenOption.CREATE_NEW;
 import static java.nio.file.StandardOpenOption.WRITE;
 
 /**
@@ -35,55 +36,77 @@ public class StateLifeCycleListener implements LifeCycle.Listener
 {
     private static final Logger LOG = LoggerFactory.getLogger(StateLifeCycleListener.class);
 
-    private final Path _filename;
+    private final Path stateFile;
 
-    public StateLifeCycleListener(String filename)
+    public StateLifeCycleListener(String filename) throws IOException
     {
-        _filename = Paths.get(filename).toAbsolutePath();
+        stateFile = Paths.get(filename).toAbsolutePath();
 
         if (LOG.isDebugEnabled())
-            LOG.debug("State File: {}", _filename);
+            LOG.debug("State File: {}", stateFile);
+
+        // We use raw Files APIs here to allow important IOExceptions
+        // to fail the startup of Jetty, as these kinds of errors
+        // point to filesystem permission issues that must be resolved
+        // by the user before the state file can be used.
+
+        // Start with fresh file (for permission reasons)
+        Files.deleteIfExists(stateFile);
+
+        // Create file
+        Files.writeString(stateFile, "INIT " + this + "\n", UTF_8, WRITE, CREATE_NEW);
     }
 
-    private void writeState(String action, LifeCycle lifecycle)
+    private void appendStateChange(String action, Object obj)
     {
-        try (Writer out = Files.newBufferedWriter(_filename, UTF_8, WRITE, CREATE, APPEND))
+        try (Writer out = Files.newBufferedWriter(stateFile, UTF_8, WRITE, APPEND))
         {
-            out.append(action).append(" ").append(lifecycle.toString()).append("\n");
+            String entry = String.format("%s %s\n", action, obj);
+            if (LOG.isDebugEnabled())
+                LOG.debug("appendEntry to {}: {}", stateFile, entry);
+            out.append(entry);
         }
-        catch (Exception e)
+        catch (IOException e)
         {
-            LOG.warn("Unable to write state", e);
+            // this can happen if the uid of the Jetty process changes after it has been started
+            // such as can happen with some setuid configurations
+            LOG.warn("Unable to append to state file: " + stateFile, e);
         }
     }
 
     @Override
     public void lifeCycleStarting(LifeCycle event)
     {
-        writeState("STARTING", event);
+        appendStateChange("STARTING", event);
     }
 
     @Override
     public void lifeCycleStarted(LifeCycle event)
     {
-        writeState("STARTED", event);
+        appendStateChange("STARTED", event);
     }
 
     @Override
     public void lifeCycleFailure(LifeCycle event, Throwable cause)
     {
-        writeState("FAILED", event);
+        appendStateChange("FAILED", event);
     }
 
     @Override
     public void lifeCycleStopping(LifeCycle event)
     {
-        writeState("STOPPING", event);
+        appendStateChange("STOPPING", event);
     }
 
     @Override
     public void lifeCycleStopped(LifeCycle event)
     {
-        writeState("STOPPED", event);
+        appendStateChange("STOPPED", event);
+    }
+
+    @Override
+    public String toString()
+    {
+        return String.format("%s@%h", this.getClass().getSimpleName(), this);
     }
 }

tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java

@@ -16,6 +16,7 @@ package org.eclipse.jetty.tests.distribution;
 import java.io.BufferedWriter;
 import java.io.File;
 import java.io.FileWriter;
+import java.io.IOException;
 import java.net.URI;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
@@ -24,6 +25,7 @@ import java.nio.file.Paths;
 import java.nio.file.StandardCopyOption;
 import java.nio.file.StandardOpenOption;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Queue;
@@ -49,6 +51,7 @@ import org.eclipse.jetty.unixsocket.client.HttpClientTransportOverUnixSockets;
 import org.eclipse.jetty.unixsocket.server.UnixSocketConnector;
 import org.eclipse.jetty.util.BlockingArrayQueue;
 import org.eclipse.jetty.util.IO;
+import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.eclipse.jetty.websocket.api.Session;
 import org.eclipse.jetty.websocket.api.StatusCode;
@@ -72,6 +75,7 @@ import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.startsWith;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
@@ -109,6 +113,78 @@ public class DistributionTests extends AbstractJettyHomeTest
         }
     }
 
+    @Test
+    public void testJettyConf() throws Exception
+    {
+        String jettyVersion = System.getProperty("jettyVersion");
+        JettyHomeTester distribution = JettyHomeTester.Builder.newInstance()
+            .jettyVersion(jettyVersion)
+            .mavenLocalRepository(System.getProperty("mavenRepoPath"))
+            .build();
+
+        try (JettyHomeTester.Run run1 = distribution.start("--add-modules=http"))
+        {
+            assertTrue(run1.awaitFor(10, TimeUnit.SECONDS));
+            assertEquals(0, run1.getExitValue());
+
+            Path pidfile = run1.getConfig().getJettyBase().resolve("jetty.pid");
+            Path statefile = run1.getConfig().getJettyBase().resolve("jetty.state");
+
+            int port = distribution.freePort();
+
+            List<String> args = new ArrayList<>();
+            args.add("jetty.http.port=" + port);
+            args.add("jetty.state=" + statefile);
+            args.add("jetty.pid=" + pidfile);
+
+            Path confFile = run1.getConfig().getJettyHome().resolve("etc/jetty.conf");
+            for (String line : Files.readAllLines(confFile, StandardCharsets.UTF_8))
+            {
+                if (line.startsWith("#") || StringUtil.isBlank(line))
+                    continue; // skip
+                args.add(line);
+            }
+
+            try (JettyHomeTester.Run run2 = distribution.start(args))
+            {
+                assertTrue(run2.awaitConsoleLogsFor("Started Server@", 10, TimeUnit.SECONDS));
+
+                assertTrue(Files.isRegularFile(pidfile), "PID file should exist");
+                assertTrue(Files.isRegularFile(statefile), "State file should exist");
+                String state = tail(statefile);
+                assertThat("State file", state, startsWith("STARTED "));
+
+                startHttpClient();
+                ContentResponse response = client.GET("http://localhost:" + port);
+                assertEquals(HttpStatus.NOT_FOUND_404, response.getStatus());
+            }
+
+            await().atMost(Duration.ofSeconds(10)).until(() -> !Files.exists(pidfile));
+            await().atMost(Duration.ofSeconds(10)).until(() -> tail(statefile).startsWith("STOPPED "));
+        }
+    }
+
+    /**
+     * Get the last line of the file.
+     *
+     * @param file the file to read from
+     * @return the string representing the last line of the file, or null if not found
+     */
+    private static String tail(Path file)
+    {
+        try
+        {
+            List<String> lines = Files.readAllLines(file, StandardCharsets.UTF_8);
+            if (lines.isEmpty())
+                return "";
+            return lines.get(lines.size() - 1);
+        }
+        catch (IOException e)
+        {
+            return "";
+        }
+    }
+
     @ParameterizedTest
     @ValueSource(booleans = {true, false})
     public void testPidFile(boolean includeJettyPidConfig) throws Exception
@@ -1270,7 +1346,6 @@ public class DistributionTests extends AbstractJettyHomeTest
                 assertThat(response.getStatus(), is(HttpStatus.OK_200));
                 content = response.getContentAsString();
                 assertThat(content, containsString("not authenticated"));
-
             }
         }
         finally
@@ -1308,7 +1383,7 @@ public class DistributionTests extends AbstractJettyHomeTest
             int port = distribution.freePort();
             String[] args2 = {
                 "jetty.http.port=" + port,
-            };
+                };
             try (JettyHomeTester.Run run2 = distribution.start(args2))
             {
                 assertTrue(run2.awaitConsoleLogsFor("Started Server@", 10, TimeUnit.SECONDS));
@@ -1324,7 +1399,7 @@ public class DistributionTests extends AbstractJettyHomeTest
 
                 Path requestLog = distribution.getJettyBase().resolve("logs/test.request.log");
                 List<String> loggedLines = Files.readAllLines(requestLog, StandardCharsets.UTF_8);
-                for (String loggedLine: loggedLines)
+                for (String loggedLine : loggedLines)
                 {
                     assertThat(loggedLine, containsString(" [foo space here] "));
                 }