From 4af93b5e19120d0b1ccbca2a9e5269871f4635cd Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 13 Aug 2021 17:32:53 +0200 Subject: [PATCH] Issue #6476 - warn on exec (#6597) Fixes #6476 - Show message if JVM args are present but new JVM is spawned * Improved documentation by correctly redacting out `jetty-halt.xml`, an XML file that is only necessary for rendering the documentation. * Added WARN message when new JVM is spawned. * Updated documentation. * Updated --list-config to report whether a JVM is forked. * Added test case. Signed-off-by: Simone Bordet --- .../jetty/docs/JettyIncludeExtension.java | 11 +++++- .../operations-guide/modules/modules.adoc | 2 + .../start/start-configure.adoc | 38 +++++++++++++++++-- .../java/org/eclipse/jetty/start/Main.java | 10 +++++ .../org/eclipse/jetty/start/StartArgs.java | 16 +++----- .../tests/distribution/DistributionTests.java | 33 ++++++++++++++++ 6 files changed, 94 insertions(+), 16 deletions(-) diff --git a/documentation/jetty-asciidoctor-extensions/src/main/java/org/eclipse/jetty/docs/JettyIncludeExtension.java b/documentation/jetty-asciidoctor-extensions/src/main/java/org/eclipse/jetty/docs/JettyIncludeExtension.java index f9ace37ef6d7..903f6920a5eb 100644 --- a/documentation/jetty-asciidoctor-extensions/src/main/java/org/eclipse/jetty/docs/JettyIncludeExtension.java +++ b/documentation/jetty-asciidoctor-extensions/src/main/java/org/eclipse/jetty/docs/JettyIncludeExtension.java @@ -144,6 +144,7 @@ private String captureOutput(Document document, Map attributes, .map(line -> redact(line, run.getConfig().getMavenLocalRepository(), "/path/to/maven.repository")) .map(line -> redact(line, run.getConfig().getJettyHome().toString(), "/path/to/jetty.home")) .map(line -> redact(line, run.getConfig().getJettyBase().toString(), "/path/to/jetty.base")) + .map(line -> regexpRedact(line, "(^| )[^ ]+/etc/jetty-halt\\.xml", "")) .map(line -> redact(line, (String)document.getAttribute("project-version"), (String)document.getAttribute("version"))); lines = replace(lines, (String)attributes.get("replace")); lines = delete(lines, (String)attributes.get("delete")); @@ -160,6 +161,13 @@ private String redact(String line, String target, String replacement) return line; } + private String regexpRedact(String line, String regexp, String replacement) + { + if (regexp != null && replacement != null) + return line.replaceAll(regexp, replacement); + return line; + } + private Stream replace(Stream lines, String replace) { if (replace == null) @@ -178,8 +186,7 @@ private Stream delete(Stream lines, String delete) if (delete == null) return lines; Pattern regExp = Pattern.compile(delete); - return lines.filter(line -> !regExp.matcher(line).find()) - .filter(line -> !line.contains("jetty-halt.xml")); + return lines.filter(line -> !regExp.matcher(line).find()); } private Stream denoteLineStart(Stream lines) diff --git a/documentation/jetty-documentation/src/main/asciidoc/operations-guide/modules/modules.adoc b/documentation/jetty-documentation/src/main/asciidoc/operations-guide/modules/modules.adoc index 67c414717672..2b020bac4bb2 100644 --- a/documentation/jetty-documentation/src/main/asciidoc/operations-guide/modules/modules.adoc +++ b/documentation/jetty-documentation/src/main/asciidoc/operations-guide/modules/modules.adoc @@ -396,6 +396,8 @@ When the `[exec]` section is present, the JVM running the Jetty start mechanism This is necessary because JVM options such as `-Xmx` (that specifies the max JVM heap size) cannot be changed in a running JVM. For an example, see xref:og-start-configure-custom-module-exec[this section]. +You can avoid that the Jetty start mechanism forks the second JVM, as explained in xref:og-start-configure-dry-run[this section]. + [[og-modules-directive-jpms]] ===== [jpms] diff --git a/documentation/jetty-documentation/src/main/asciidoc/operations-guide/start/start-configure.adoc b/documentation/jetty-documentation/src/main/asciidoc/operations-guide/start/start-configure.adoc index c6814ce45d63..e711d9018eae 100644 --- a/documentation/jetty-documentation/src/main/asciidoc/operations-guide/start/start-configure.adoc +++ b/documentation/jetty-documentation/src/main/asciidoc/operations-guide/start/start-configure.adoc @@ -163,10 +163,15 @@ $ java -jar $JETTY_HOME/start.jar --add-modules=jvm Since the module defines an `[exec]` section, it will fork _another_ JVM when Jetty is started. -This means that when you start Jetty, there will be _two_ JVMs running: one spawned by you when you run `java -jar $JETTY_HOME/start.jar`, and another spawned by the Jetty start mechanism with the JVM options you specified (that cannot be applied to an already running JVM). +This means that when you start Jetty, there will be _two_ JVMs running: one created by you when you run `java -jar $JETTY_HOME/start.jar`, and another forked by the Jetty start mechanism with the JVM options you specified (that cannot be applied to an already running JVM). Again, you can xref:og-start-configure-dry-run[display the JVM command line] to verify that it is correct. +[TIP] +==== +The second JVM forked by the Jetty start mechanism when one of the modules requires forking, for example a module that contains an `[exec]` section, may not be desirable, and may be avoided as explained in xref:og-start-configure-dry-run[this section]. +==== + [[og-start-configure-display]] ===== Displaying the Configuration @@ -205,7 +210,28 @@ Some option, such as `--jpms`, imply `--exec`, as it won't be possible to modify To start Jetty without forking a second JVM, the `--dry-run` option can be used to generate a command line that is then executed so that starting Jetty only spawns one JVM. -The `--dry-run` option is quite flexible and below you can find a few examples of how to use it to generate scripts or to create an arguments file that can be passed to the `java` executable. +IMPORTANT: You can use the `--dry-run` option as explained below to avoid forking a second JVM when using modules that have the `[exec]` section, or the `--exec` option, or when using the `--jpms` option. + +For example, using the `--dry-run` option with the `jvm.mod` introduced in xref:og-start-configure-custom-module-exec[this section] produces the following command line: + +---- +$ java -jar $JETTY_HOME/start.jar --dry-run +---- + +[source,options=nowrap] +---- +include::jetty[setupModules="src/main/asciidoc/operations-guide/start/jvm.mod",setupArgs="--add-modules=http,jvm",args="--dry-run",replace="( ),$1\\\n"] +---- + +You can then run the generated command line. + +For example, in the Linux `bash` shell you can run it by wrapping it into `$(\...)`: + +---- +$ $(java -jar $JETTY_HOME/start.jar --dry-run) +---- + +The `--dry-run` option is quite flexible and below you can find a few examples of how to use it to avoid forking a second JVM, or generating scripts or creating an arguments file that can be passed to (a possibly alternative) `java` executable. To display the `java` executable used to start Jetty: @@ -304,7 +330,13 @@ $ java -jar $JETTY_HOME/start.jar --dry-run=##opts,path,main,args## > /tmp/jvm_c $ /some/other/java @/tmp/jvm_cmd_line.txt ---- -Alternatively, they can be combined in a shell script: +Using `--dry-run=opts,path,main,args` can be used to avoid that the Jetty start mechanism forks a second JVM when using modules that require forking: + +---- +$ java $(java -jar $JETTY_HOME/start.jar --dry-run=opts,path,main,args) +---- + +The output of different `--dry-run` executions can be creatively combined in a shell script: [source,subs=quotes] ---- diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java index ac56b37ea50e..82c7ad3df4a5 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java @@ -30,6 +30,7 @@ import java.nio.file.Path; import java.util.List; import java.util.Locale; +import java.util.stream.Collectors; import org.eclipse.jetty.start.Props.Prop; import org.eclipse.jetty.start.config.CommandLineConfigSource; @@ -467,6 +468,15 @@ else if (args.isCreateFiles() || !args.getStartModules().isEmpty()) { CommandLineBuilder cmd = args.getMainArgs(StartArgs.ALL_PARTS); cmd.debug(); + + List execModules = args.getEnabledModules().stream() + .map(name -> args.getAllModules().get(name)) + // Keep only the forking modules. + .filter(module -> !module.getJvmArgs().isEmpty()) + .map(Module::getName) + .collect(Collectors.toList()); + StartLog.warn("Forking second JVM due to forking module(s): %s. Use --dry-run to generate the command line to avoid forking.", execModules); + ProcessBuilder pbuilder = new ProcessBuilder(cmd.getArgs()); StartLog.endStartLog(); final Process process = pbuilder.start(); diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java b/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java index b9d3fb0298d2..812610fc1998 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java @@ -302,7 +302,7 @@ public void dumpEnvironment() // Jetty Environment System.out.println(); System.out.println("Jetty Environment:"); - System.out.println("-----------------"); + System.out.println("------------------"); dumpProperty(JETTY_VERSION_KEY); dumpProperty(JETTY_TAG_NAME_KEY); dumpProperty(JETTY_BUILDNUM_KEY); @@ -330,26 +330,20 @@ public void dumpEnvironment() public void dumpJvmArgs() { - System.out.println(); - System.out.println("JVM Arguments:"); - System.out.println("--------------"); if (jvmArgs.isEmpty()) - { - System.out.println(" (no jvm args specified)"); return; - } + + System.out.println(); + System.out.println("Forked JVM Arguments:"); + System.out.println("---------------------"); for (String jvmArgKey : jvmArgs) { String value = System.getProperty(jvmArgKey); if (value != null) - { System.out.printf(" %s = %s%n", jvmArgKey, value); - } else - { System.out.printf(" %s%n", jvmArgKey); - } } } diff --git a/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java b/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java index 089d539fe79b..41129f76dcb7 100644 --- a/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java +++ b/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java @@ -931,4 +931,37 @@ public void testUnixDomain() throws Exception } } } + + @Test + public void testModuleWithExecEmitsWarning() throws Exception + { + String jettyVersion = System.getProperty("jettyVersion"); + JettyHomeTester distribution = JettyHomeTester.Builder.newInstance() + .jettyVersion(jettyVersion) + .mavenLocalRepository(System.getProperty("mavenRepoPath")) + .build(); + + Path jettyBase = distribution.getJettyBase(); + Path jettyBaseModules = jettyBase.resolve("modules"); + Files.createDirectories(jettyBaseModules); + Path execModule = jettyBaseModules.resolve("exec.mod"); + String module = "" + + "[exec]\n" + + "--show-version"; + Files.write(execModule, List.of(module), StandardOpenOption.CREATE); + + try (JettyHomeTester.Run run1 = distribution.start(List.of("--add-modules=http,exec"))) + { + assertTrue(run1.awaitFor(10, TimeUnit.SECONDS)); + assertEquals(0, run1.getExitValue()); + + int port = distribution.freePort(); + try (JettyHomeTester.Run run2 = distribution.start("jetty.http.port=" + port)) + { + assertTrue(run2.awaitConsoleLogsFor("Started Server@", 10, TimeUnit.SECONDS)); + assertTrue(run2.getLogs().stream() + .anyMatch(log -> log.contains("WARN") && log.contains("Forking"))); + } + } + } }