From b0db0a847eff713fda3852622fe9c016baa548c7 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 15 Aug 2014 09:56:44 -0400 Subject: [PATCH 1/5] Scalariform reformat of JUnitXmlTestsListener.scala --- testing/src/main/scala/sbt/JUnitXmlTestsListener.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala b/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala index ec9303283..6403b38eb 100644 --- a/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala +++ b/testing/src/main/scala/sbt/JUnitXmlTestsListener.scala @@ -1,7 +1,7 @@ package sbt -import java.io.{IOException, StringWriter, PrintWriter, File} -import java.net.{InetAddress} +import java.io.{ IOException, StringWriter, PrintWriter, File } +import java.net.{ InetAddress } import scala.collection.mutable.ListBuffer import scala.util.DynamicVariable From 3a065dc047f765cc7ae4d0ff4f881c424613265d Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 15 Aug 2014 11:07:19 -0400 Subject: [PATCH 2/5] Set Java memory options for server applications The defaults here are copied from Activator which in turn are probably copied from somewhere else. They are of course basically arbitrary, but the JVM's defaults if we don't set memory options are insufficient to run most apps. If the user sets any memory options in the configuration, we completely leave them alone and don't set any of our own. So it's always possible to override. --- .../scala/xsbt/boot/ServerApplication.scala | 62 +++++++++++++++++-- launch/src/test/scala/ServerLocatorTest.scala | 39 +++++++++++- 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/launch/src/main/scala/xsbt/boot/ServerApplication.scala b/launch/src/main/scala/xsbt/boot/ServerApplication.scala index b1d90fe38..bc0be9780 100644 --- a/launch/src/main/scala/xsbt/boot/ServerApplication.scala +++ b/launch/src/main/scala/xsbt/boot/ServerApplication.scala @@ -136,10 +136,7 @@ object ServerLauncher { if (System.getenv("SBT_SERVER_SAVE_TEMPS") eq null) launchConfig.deleteOnExit() LaunchConfiguration.save(config, launchConfig) - val jvmArgs: List[String] = serverConfig.jvmArgs map readLines match { - case Some(args) => args - case None => Nil - } + val jvmArgs: List[String] = serverJvmArgs(currentDirectory, serverConfig) val cmd: List[String] = ("java" :: jvmArgs) ++ ("-jar" :: defaultLauncherLookup.getCanonicalPath :: s"@load:${launchConfig.toURI.toURL.toString}" :: Nil) @@ -211,6 +208,63 @@ object ServerLauncher { finally reader.close() } + // None = couldn't figure it out + def javaIsAbove(currentDirectory: File, version: Int): Option[Boolean] = { + val pb = new java.lang.ProcessBuilder() + // hopefully "java -version" is a lot faster than booting the full JVM. + // not sure how else we can do this. + pb.command("java", "-version") + pb.directory(currentDirectory) + val process = pb.start() + try { + process.getOutputStream.close() + process.getInputStream.close() + val stderr = new java.io.LineNumberReader(new java.io.InputStreamReader(process.getErrorStream)) + // Looking for the first line which is `java version "1.7.0_60"` or similar + val lineOption = try Option(stderr.readLine()) finally stderr.close() + val re = """java version "[0-9]+\.([0-9]+)\..*".*""".r + lineOption flatMap { + case re(v) => try Some(Integer.parseInt(v) > version) catch { case NonFatal(_) => None } + case other => None + } + } finally { + process.destroy() + try { process.waitFor() } catch { case NonFatal(_) => } + } + } + + def serverJvmArgs(currentDirectory: File, serverConfig: ServerConfiguration): List[String] = + serverJvmArgs(currentDirectory, serverConfig.jvmArgs map readLines getOrElse Nil) + + final val memOptPrefixes = List("-Xmx", "-Xms", "-XX:MaxPermSize", "-XX:PermSize", "-XX:ReservedCodeCacheSize", "-XX:MaxMetaspaceSize", "-XX:MetaspaceSize") + + final val defaultMinHeapM = 256 + final val defaultMaxHeapM = defaultMinHeapM * 4 + final val defaultMinPermM = 64 + final val defaultMaxPermM = defaultMinPermM * 4 + + // this is separate just for the test suite + def serverJvmArgs(currentDirectory: File, baseArgs: List[String]): List[String] = { + // ignore blank lines + val trimmed = baseArgs.map(_.trim).filterNot(_.isEmpty) + // If the user config has provided ANY memory options we bail out and do NOT add + // any defaults. This means people can always fix our mistakes, and it avoids + // issues where the JVM refuses to start because of (for example) min size greater + // than max size. We don't want to deal with coordinating our changes with the + // user configuration. + def isMemoryOption(s: String) = memOptPrefixes.exists(s.startsWith(_)) + if (trimmed.exists(isMemoryOption(_))) + trimmed + else { + val permOptions = javaIsAbove(currentDirectory, 7) match { + case Some(true) => List(s"-XX:MetaspaceSize=${defaultMinPermM}m", s"-XX:MaxMetaspaceSize=${defaultMaxPermM}m") + case Some(false) => List(s"-XX:PermSize=${defaultMinPermM}m", s"-XX:MaxPermSize=${defaultMaxPermM}m") + case None => Nil // don't know what we're doing, so don't set options + } + s"-Xms${defaultMinHeapM}m" :: s"-Xmx${defaultMaxHeapM}m" :: (permOptions ++ trimmed) + } + } + def defaultLauncherLookup: File = try { val classInLauncher = classOf[AppConfiguration] diff --git a/launch/src/test/scala/ServerLocatorTest.scala b/launch/src/test/scala/ServerLocatorTest.scala index 2ad80a14b..82a76f14e 100644 --- a/launch/src/test/scala/ServerLocatorTest.scala +++ b/launch/src/test/scala/ServerLocatorTest.scala @@ -48,5 +48,42 @@ object ServerLocatorTest extends Specification { finally inputStream.close() result must equalTo(Some(expected)) } + "determine a JVM version" in { + withTemporaryDirectory { dir => + // javaIs8OrAbove returns None for pathological situations + // (weird errors running java -version or something), + // but when testing sbt we should not be in such a situation. + val determined = ServerLauncher.javaIsAbove(dir, 7) + determined must beSome + } + } + "have JVM memory defaults" in { + withTemporaryDirectory { dir => + val defaults = ServerLauncher.serverJvmArgs(dir, Nil) + defaults must contain(beEqualTo("-Xms256m")) + defaults must contain(beEqualTo("-Xmx1024m")) + if (ServerLauncher.javaIsAbove(dir, 7).getOrElse(false)) { + defaults must contain(beEqualTo("-XX:MetaspaceSize=64m")) + defaults must contain(beEqualTo("-XX:MaxMetaspaceSize=256m")) + } else { + defaults must contain(beEqualTo("-XX:PermSize=64m")) + defaults must contain(beEqualTo("-XX:MaxPermSize=256m")) + } + } + } + "leave user-specified memory options alone" in { + withTemporaryDirectory { dir => + val args = ServerLauncher.serverJvmArgs(dir, List("-Xmx4321m")) + args must contain(beEqualTo("-Xmx4321m")) + args must not contain (beEqualTo("-Xms256m")) + args must not contain (beEqualTo("-Xmx1024m")) + } + } + "ignore whitespace in jvm args file" in { + withTemporaryDirectory { dir => + val args = ServerLauncher.serverJvmArgs(dir, List("", " ", " -Xmx4321m ", " ", "")) + args must equalTo(List("-Xmx4321m")) + } + } } -} \ No newline at end of file +} From 081f6bac077243fd3d51514d52b80c7a478f9554 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 15 Aug 2014 15:20:50 -0400 Subject: [PATCH 3/5] Ignore "java -version" IOException when launching servers process.start and reading from the stderr stream could in theory throw IOException, we want to return None then rather than crashing. --- launch/src/main/scala/xsbt/boot/ServerApplication.scala | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/launch/src/main/scala/xsbt/boot/ServerApplication.scala b/launch/src/main/scala/xsbt/boot/ServerApplication.scala index bc0be9780..c8123bcb9 100644 --- a/launch/src/main/scala/xsbt/boot/ServerApplication.scala +++ b/launch/src/main/scala/xsbt/boot/ServerApplication.scala @@ -209,7 +209,7 @@ object ServerLauncher { } // None = couldn't figure it out - def javaIsAbove(currentDirectory: File, version: Int): Option[Boolean] = { + def javaIsAbove(currentDirectory: File, version: Int): Option[Boolean] = try { val pb = new java.lang.ProcessBuilder() // hopefully "java -version" is a lot faster than booting the full JVM. // not sure how else we can do this. @@ -231,6 +231,11 @@ object ServerLauncher { process.destroy() try { process.waitFor() } catch { case NonFatal(_) => } } + } catch { + case e: IOException => + // both process.start and reading the output streams can throw IOException. + // all OS exceptions from process.start are supposed to be IOException. + None } def serverJvmArgs(currentDirectory: File, serverConfig: ServerConfiguration): List[String] = From d7ca9c5637f488ced21788bde9df15fcf0c6644a Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 15 Aug 2014 15:24:21 -0400 Subject: [PATCH 4/5] Server launcher: Print errors running java -version to stderr We forward errors from the spawned server to stderr, so we are already using stderr and this should save someone some pain vs. just swallowing the fail. --- launch/src/main/scala/xsbt/boot/ServerApplication.scala | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/launch/src/main/scala/xsbt/boot/ServerApplication.scala b/launch/src/main/scala/xsbt/boot/ServerApplication.scala index c8123bcb9..9194e1ff1 100644 --- a/launch/src/main/scala/xsbt/boot/ServerApplication.scala +++ b/launch/src/main/scala/xsbt/boot/ServerApplication.scala @@ -225,7 +225,9 @@ object ServerLauncher { val re = """java version "[0-9]+\.([0-9]+)\..*".*""".r lineOption flatMap { case re(v) => try Some(Integer.parseInt(v) > version) catch { case NonFatal(_) => None } - case other => None + case other => + System.err.println(s"Failed to parse version from 'java -version' output '$other'") + None } } finally { process.destroy() @@ -235,6 +237,7 @@ object ServerLauncher { case e: IOException => // both process.start and reading the output streams can throw IOException. // all OS exceptions from process.start are supposed to be IOException. + System.err.println(s"Failed to run 'java -version': ${e.getClass.getName}: ${e.getMessage}") None } From 705b60dc1143b2e62326e67bbf25bdcee90843b2 Mon Sep 17 00:00:00 2001 From: Havoc Pennington Date: Fri, 15 Aug 2014 15:34:11 -0400 Subject: [PATCH 5/5] Use java Pattern directly in launcher instead of Scala API This keeps the launch jar smaller --- .../main/scala/xsbt/boot/ServerApplication.scala | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/launch/src/main/scala/xsbt/boot/ServerApplication.scala b/launch/src/main/scala/xsbt/boot/ServerApplication.scala index 9194e1ff1..8ac87c4d7 100644 --- a/launch/src/main/scala/xsbt/boot/ServerApplication.scala +++ b/launch/src/main/scala/xsbt/boot/ServerApplication.scala @@ -222,12 +222,15 @@ object ServerLauncher { val stderr = new java.io.LineNumberReader(new java.io.InputStreamReader(process.getErrorStream)) // Looking for the first line which is `java version "1.7.0_60"` or similar val lineOption = try Option(stderr.readLine()) finally stderr.close() - val re = """java version "[0-9]+\.([0-9]+)\..*".*""".r - lineOption flatMap { - case re(v) => try Some(Integer.parseInt(v) > version) catch { case NonFatal(_) => None } - case other => - System.err.println(s"Failed to parse version from 'java -version' output '$other'") + val pattern = java.util.regex.Pattern.compile("""java version "[0-9]+\.([0-9]+)\..*".*""") + lineOption flatMap { line => + val matcher = pattern.matcher(line) + if (matcher.matches()) { + try Some(Integer.parseInt(matcher.group(1)) > version) catch { case NonFatal(_) => None } + } else { + System.err.println(s"Failed to parse version from 'java -version' output '$line'") None + } } } finally { process.destroy()