Remove manual parsing of JVM options

distribution/src/bin/elasticsearch

@@ -18,7 +18,7 @@ source "`dirname "$0"`"/elasticsearch-env
 
 ES_JVM_OPTIONS="$ES_PATH_CONF"/jvm.options
 JVM_OPTIONS=`"$JAVA" -cp "$ES_CLASSPATH" org.elasticsearch.tools.launchers.JvmOptionsParser "$ES_JVM_OPTIONS"`
-ES_JAVA_OPTS="${JVM_OPTIONS//\$\{ES_TMPDIR\}/$ES_TMPDIR} $ES_JAVA_OPTS"
+ES_JAVA_OPTS="${JVM_OPTIONS//\$\{ES_TMPDIR\}/$ES_TMPDIR}"
 
 # manual parsing to find out, if process should be detached
 if ! echo $* | grep -E '(^-d |-d$| -d |--daemonize$|--daemonize )' > /dev/null; then

distribution/src/bin/elasticsearch-service.bat

@@ -112,7 +112,7 @@ if not "%ES_JAVA_OPTS%" == "" set ES_JAVA_OPTS=%ES_JAVA_OPTS: =;%
 
 @setlocal
 for /F "usebackq delims=" %%a in (`"%JAVA% -cp "!ES_CLASSPATH!" "org.elasticsearch.tools.launchers.JvmOptionsParser" "!ES_JVM_OPTIONS!" || echo jvm_options_parser_failed"`) do set JVM_OPTIONS=%%a
-@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!% %ES_JAVA_OPTS%
+@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!%
 
 if "%MAYBE_JVM_OPTIONS_PARSER_FAILED%" == "jvm_options_parser_failed" (
   exit /b 1

distribution/src/bin/elasticsearch.bat

@@ -44,7 +44,7 @@ IF ERRORLEVEL 1 (
 set ES_JVM_OPTIONS=%ES_PATH_CONF%\jvm.options
 @setlocal
 for /F "usebackq delims=" %%a in (`CALL %JAVA% -cp "!ES_CLASSPATH!" "org.elasticsearch.tools.launchers.JvmOptionsParser" "!ES_JVM_OPTIONS!" ^|^| echo jvm_options_parser_failed`) do set JVM_OPTIONS=%%a
-@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!% %ES_JAVA_OPTS%
+@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!%
 
 if "%MAYBE_JVM_OPTIONS_PARSER_FAILED%" == "jvm_options_parser_failed" (
   exit /b 1

distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmErgonomics.java

@@ -19,24 +19,27 @@
 
 package org.elasticsearch.tools.launchers;
 
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Optional;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
  * Tunes Elasticsearch JVM settings based on inspection of provided JVM options.
  */
 final class JvmErgonomics {
-    private static final long KB = 1024L;
-
-    private static final long MB = 1024L * 1024L;
-
-    private static final long GB = 1024L * 1024L * 1024L;
-
 
     private JvmErgonomics() {
         throw new AssertionError("No instances intended");
@@ -48,48 +51,73 @@ private JvmErgonomics() {
      * @param userDefinedJvmOptions A list of JVM options that have been defined by the user.
      * @return A list of additional JVM options to set.
      */
-    static List<String> choose(List<String> userDefinedJvmOptions) {
-        List<String> ergonomicChoices = new ArrayList<>();
-        Long heapSize = extractHeapSize(userDefinedJvmOptions);
-        Map<String, String> systemProperties = extractSystemProperties(userDefinedJvmOptions);
-        if (heapSize != null) {
-            if (systemProperties.containsKey("io.netty.allocator.type") == false) {
-                if (heapSize <= 1 * GB) {
-                    ergonomicChoices.add("-Dio.netty.allocator.type=unpooled");
-                } else {
-                    ergonomicChoices.add("-Dio.netty.allocator.type=pooled");
-                }
+    static List<String> choose(final List<String> userDefinedJvmOptions) throws InterruptedException, IOException {
+        final List<String> ergonomicChoices = new ArrayList<>();
+        final Map<String, Optional<String>> finalJvmOptions = finalJvmOptions(userDefinedJvmOptions);
+        final long heapSize = extractHeapSize(finalJvmOptions);
+        final Map<String, String> systemProperties = extractSystemProperties(userDefinedJvmOptions);
+        if (systemProperties.containsKey("io.netty.allocator.type") == false) {
+            if (heapSize <= 1 << 30) {
+                ergonomicChoices.add("-Dio.netty.allocator.type=unpooled");
+            } else {
+                ergonomicChoices.add("-Dio.netty.allocator.type=pooled");
             }
         }
         return ergonomicChoices;
     }
 
-    private static final Pattern MAX_HEAP_SIZE = Pattern.compile("^(-Xmx|-XX:MaxHeapSize=)(?<size>\\d+)(?<unit>\\w)?$");
+    private static final Pattern OPTION =
+            Pattern.compile("^\\s*\\S+\\s+(?<flag>\\S+)\\s+:?=\\s+(?<value>\\S+)?\\s+\\{[^}]+?\\}\\s+\\{[^}]+}");
 
-    // package private for testing
-    static Long extractHeapSize(List<String> userDefinedJvmOptions) {
-        for (String jvmOption : userDefinedJvmOptions) {
-            final Matcher matcher = MAX_HEAP_SIZE.matcher(jvmOption);
-            if (matcher.matches()) {
-                final long size = Long.parseLong(matcher.group("size"));
-                final String unit = matcher.group("unit");
-                if (unit == null) {
-                    return size;
-                } else {
-                    switch (unit.toLowerCase(Locale.ROOT)) {
-                        case "k":
-                            return size * KB;
-                        case "m":
-                            return size * MB;
-                        case "g":
-                            return size * GB;
-                        default:
-                            throw new IllegalArgumentException("Unknown unit [" + unit + "] for max heap size in [" + jvmOption + "]");
-                    }
-                }
-            }
+    static Map<String, Optional<String>> finalJvmOptions(
+            final List<String> userDefinedJvmOptions) throws InterruptedException, IOException {
+        return flagsFinal(userDefinedJvmOptions).stream()
+                .map(OPTION::matcher).filter(Matcher::matches)
+                .collect(Collectors.toUnmodifiableMap(m -> m.group("flag"), m -> Optional.ofNullable(m.group("value"))));
+    }
+
+    private static List<String> flagsFinal(final List<String> userDefinedJvmOptions) throws InterruptedException, IOException {
+        /*
+         * To deduce the final set of JVM options that Elasticsearch is going to start with, we start a separate Java process with the JVM
+         * options that we would pass on the command line. For this Java process we will add two additional flags, -XX:+PrintFlagsFinal and
+         * -version. This causes the Java process that we start to parse the JVM options into their final values, display them on standard
+         * output, print the version to standard error, and then exit. The JVM itself never bootstraps, and therefore this process is
+         * lightweight. By doing this, we get the JVM options parsed exactly as the JVM that we are going to execute would parse them
+         * without having to implement our own JVM option parsing logic.
+         */
+        final String java = Path.of(System.getProperty("java.home"), "bin", "java").toString();
+        final List<String> command =
+                Stream.of(Stream.of(java), userDefinedJvmOptions.stream(), Stream.of("-XX:+PrintFlagsFinal"), Stream.of("-version"))
+                        .reduce(Stream::concat)
+                        .get()
+                        .collect(Collectors.toUnmodifiableList());
+        final Process process = new ProcessBuilder().command(command).start();
+        final List<String> output = readLinesFromInputStream(process.getInputStream());
+        final List<String> error = readLinesFromInputStream(process.getErrorStream());
+        final int status = process.waitFor();
+        if (status != 0) {
+            final String message = String.format(
+                    Locale.ROOT,
+                    "starting java failed with [%d]\noutput:\n%s\nerror:\n%s",
+                    status,
+                    String.join("\n", output),
+                    String.join("\n", error));
+            throw new RuntimeException(message);
+        } else {
+            return output;
+        }
+    }
+
+    private static List<String> readLinesFromInputStream(final InputStream is) throws IOException {
+        try (InputStreamReader isr = new InputStreamReader(is, StandardCharsets.UTF_8);
+             BufferedReader br = new BufferedReader(isr)) {
+            return br.lines().collect(Collectors.toUnmodifiableList());
         }
-        return null;
+    }
+
+    // package private for testing
+    static Long extractHeapSize(final Map<String, Optional<String>> finalJvmOptions) {
+        return Long.parseLong(finalJvmOptions.get("MaxHeapSize").get());
     }
 
     private static final Pattern SYSTEM_PROPERTY = Pattern.compile("^-D(?<key>[\\w+].*?)=(?<value>.*)$");
@@ -105,4 +133,5 @@ static Map<String, String> extractSystemProperties(List<String> userDefinedJvmOp
         }
         return systemProperties;
     }
+
 }

distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmOptionsParser.java

@@ -19,12 +19,14 @@
 
 package org.elasticsearch.tools.launchers;
 
+import org.elasticsearch.tools.java_version_checker.JavaVersion;
+
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.Reader;
-import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Paths;
 import java.util.ArrayList;
@@ -35,10 +37,10 @@
 import java.util.Map;
 import java.util.SortedMap;
 import java.util.TreeMap;
+import java.util.function.Predicate;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
-
-import org.elasticsearch.tools.java_version_checker.JavaVersion;
+import java.util.stream.Collectors;
 
 /**
  * Parses JVM options from a file and prints a single line with all JVM options to standard output.
@@ -51,14 +53,14 @@ final class JvmOptionsParser {
      *
      * @param args the args to the program which should consist of a single option, the path to the JVM options
      */
-    public static void main(final String[] args) throws IOException {
+    public static void main(final String[] args) throws InterruptedException, IOException {
         if (args.length != 1) {
             throw new IllegalArgumentException("expected one argument specifying path to jvm.options but was " + Arrays.toString(args));
         }
         final List<String> jvmOptions = new ArrayList<>();
         final SortedMap<Integer, String> invalidLines = new TreeMap<>();
         try (InputStream is = Files.newInputStream(Paths.get(args[0]));
-             Reader reader = new InputStreamReader(is, Charset.forName("UTF-8"));
+             Reader reader = new InputStreamReader(is, StandardCharsets.UTF_8);
              BufferedReader br = new BufferedReader(reader)) {
             parse(
                     JavaVersion.majorVersion(JavaVersion.CURRENT),
@@ -78,7 +80,14 @@ public void accept(final int lineNumber, final String line) {
         }
 
         if (invalidLines.isEmpty()) {
-            List<String> ergonomicJvmOptions = JvmErgonomics.choose(jvmOptions);
+            // now append the JVM options from ES_JAVA_OPTS
+            final String environmentJvmOptions = System.getenv("ES_JAVA_OPTS");
+            if (environmentJvmOptions != null) {
+                jvmOptions.addAll(Arrays.stream(environmentJvmOptions.split("\\s+"))
+                        .filter(Predicate.not(String::isBlank))
+                        .collect(Collectors.toUnmodifiableList()));
+            }
+            final List<String> ergonomicJvmOptions = JvmErgonomics.choose(jvmOptions);
             jvmOptions.addAll(ergonomicJvmOptions);
             final String spaceDelimitedJvmOptions = spaceDelimitJvmOptions(jvmOptions);
             Launchers.outPrintln(spaceDelimitedJvmOptions);

distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmErgonomicsTests.java

@@ -19,38 +19,70 @@
 
 package org.elasticsearch.tools.launchers;
 
+import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.hasToString;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 public class JvmErgonomicsTests extends LaunchersTestCase {
-    public void testExtractValidHeapSize() {
-        assertEquals(Long.valueOf(1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx1024")));
-        assertEquals(Long.valueOf(2L * 1024 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx2g")));
-        assertEquals(Long.valueOf(32 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx32M")));
-        assertEquals(Long.valueOf(32 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-XX:MaxHeapSize=32M")));
+
+    public void testExtractValidHeapSizeUsingXmx() throws InterruptedException, IOException {
+        assertThat(
+                JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx2g"))),
+                equalTo(2L << 30));
+    }
+
+    public void testExtractValidHeapSizeUsingMaxHeapSize() throws InterruptedException, IOException {
+        assertThat(
+                JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-XX:MaxHeapSize=2g"))),
+                equalTo(2L << 30));
+    }
+
+    public void testExtractValidHeapSizeNoOptionPresent() throws InterruptedException, IOException {
+        assertThat(
+                JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.emptyList())),
+                greaterThan(0L));
+    }
+
+    public void testHeapSizeInvalid() throws InterruptedException, IOException {
+        try {
+            JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx2Z")));
+            fail("expected starting java to fail");
+        } catch (final RuntimeException e) {
+            assertThat(e, hasToString(containsString(("starting java failed"))));
+            assertThat(e, hasToString(containsString(("Invalid maximum heap size: -Xmx2Z"))));
+        }
     }
 
-    public void testExtractInvalidHeapSize() {
+    public void testHeapSizeTooSmall() throws InterruptedException, IOException {
         try {
-            JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx2T"));
-            fail("Expected IllegalArgumentException to be raised");
-        } catch (IllegalArgumentException expected) {
-            assertEquals("Unknown unit [T] for max heap size in [-Xmx2T]", expected.getMessage());
+            JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx1024")));
+            fail("expected starting java to fail");
+        } catch (final RuntimeException e) {
+            assertThat(e, hasToString(containsString(("starting java failed"))));
+            assertThat(e, hasToString(containsString(("Too small maximum heap"))));
         }
     }
 
-    public void testExtractNoHeapSize() {
-        assertNull("No spaces allowed", JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx 1024")));
-        assertNull("JVM option is not present", JvmErgonomics.extractHeapSize(Collections.singletonList("")));
-        assertNull("Multiple JVM options per line", JvmErgonomics.extractHeapSize(Collections.singletonList("-Xms2g -Xmx2g")));
+    public void testHeapSizeWithSpace() throws InterruptedException, IOException {
+        try {
+            JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx 1024")));
+            fail("expected starting java to fail");
+        } catch (final RuntimeException e) {
+            assertThat(e, hasToString(containsString(("starting java failed"))));
+            assertThat(e, hasToString(containsString(("Invalid maximum heap size: -Xmx 1024"))));
+        }
     }
 
     public void testExtractSystemProperties() {
@@ -69,15 +101,16 @@ public void testExtractNoSystemProperties() {
         assertTrue(parsedSystemProperties.isEmpty());
     }
 
-    public void testLittleMemoryErgonomicChoices() {
+    public void testLittleMemoryErgonomicChoices() throws InterruptedException, IOException {
         String smallHeap = randomFrom(Arrays.asList("64M", "512M", "1024M", "1G"));
         List<String> expectedChoices = Collections.singletonList("-Dio.netty.allocator.type=unpooled");
         assertEquals(expectedChoices, JvmErgonomics.choose(Arrays.asList("-Xms" + smallHeap, "-Xmx" + smallHeap)));
     }
 
-    public void testPlentyMemoryErgonomicChoices() {
+    public void testPlentyMemoryErgonomicChoices() throws InterruptedException, IOException {
         String largeHeap = randomFrom(Arrays.asList("1025M", "2048M", "2G", "8G"));
         List<String> expectedChoices = Collections.singletonList("-Dio.netty.allocator.type=pooled");
         assertEquals(expectedChoices, JvmErgonomics.choose(Arrays.asList("-Xms" + largeHeap, "-Xmx" + largeHeap)));
     }
+
 }