From 8d26bc73b45fc256f1dd449e9f04df6ef5d20525 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Sat, 16 Nov 2019 11:35:56 -0800 Subject: [PATCH 1/4] Shutdown background job on error When running a main method, if the user inputs ctrl+c then the `run` task will exit but the main method is not interrupted so it continues running even once sbt has returned to the shell. If the main method is a webserver, this prevents run from ever starting again on a fixed port. To fix this, we can modify the waitForTry method to stop the job if an exception is thrown (ctrl+c leads to an interrupted exception being thrown by waitFor). I rework the BackgroundJobService so that the default implementation of waitForTry is now usable and no longer needs to be overridden. The side effect of this change is that waitFor may now throw an exception. Within sbt, waitFor was only used in one place and I reworked it to use waitForTry instead. This could theoretically break a downstream user who relied on waitFor not throwing an exception but I suspect that there aren't many users of this api, if any at all. --- .../main/scala/sbt/BackgroundJobService.scala | 21 +++++++++++++--- main/src/main/scala/sbt/Defaults.scala | 3 ++- .../DefaultBackgroundJobService.scala | 25 +++---------------- 3 files changed, 23 insertions(+), 26 deletions(-) diff --git a/main/src/main/scala/sbt/BackgroundJobService.scala b/main/src/main/scala/sbt/BackgroundJobService.scala index 1532cfbcc..1cbff3f2d 100644 --- a/main/src/main/scala/sbt/BackgroundJobService.scala +++ b/main/src/main/scala/sbt/BackgroundJobService.scala @@ -8,11 +8,14 @@ package sbt import java.io.Closeable + import sbt.util.Logger -import Def.{ ScopedKey, Classpath } +import Def.{ Classpath, ScopedKey } import sbt.internal.util.complete._ import java.io.File -import scala.util.Try + +import scala.util.control.NonFatal +import scala.util.{ Failure, Success, Try } abstract class BackgroundJobService extends Closeable { @@ -49,9 +52,19 @@ abstract class BackgroundJobService extends Closeable { def jobs: Vector[JobHandle] def stop(job: JobHandle): Unit + /** + * Delegate to waitFor but catches any exceptions and returns the result in an instance of `Try`. + * @param job the job to wait for + * @return the result of waiting for the job to complete. + */ def waitForTry(job: JobHandle): Try[Unit] = { - // This implementation is provided only for backward compatibility. - Try(waitFor(job)) + try Success(waitFor(job)) + catch { + case NonFatal(e) => + try stop(job) + catch { case NonFatal(_) => } + Failure(e) + } } def waitFor(job: JobHandle): Unit diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 2967f16a3..8605a4139 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -1599,7 +1599,8 @@ object Defaults extends BuildCommon { } def bgWaitForTask: Initialize[InputTask[Unit]] = foreachJobTask { (manager, handle) => - manager.waitFor(handle) + manager.waitForTry(handle) + () } def docTaskSettings(key: TaskKey[File] = doc): Seq[Setting[_]] = diff --git a/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala b/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala index 12b94188c..a0328c731 100644 --- a/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala +++ b/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala @@ -161,7 +161,7 @@ private[sbt] abstract class AbstractBackgroundJobService extends BackgroundJobSe jobSet.headOption.foreach { case handle: ThreadJobHandle @unchecked => handle.job.shutdown() - handle.job.awaitTermination() + handle.job.awaitTerminationTry() case _ => // } } @@ -178,24 +178,9 @@ private[sbt] abstract class AbstractBackgroundJobService extends BackgroundJobSe ) } - private def withHandleTry(job: JobHandle)(f: ThreadJobHandle => Try[Unit]): Try[Unit] = - job match { - case handle: ThreadJobHandle @unchecked => f(handle) - case _: DeadHandle @unchecked => Try(()) // nothing to stop or wait for - case other => - Try( - sys.error( - s"BackgroundJobHandle does not originate with the current BackgroundJobService: $other" - ) - ) - } - override def stop(job: JobHandle): Unit = withHandle(job)(_.job.shutdown()) - override def waitForTry(job: JobHandle): Try[Unit] = - withHandleTry(job)(_.job.awaitTerminationTry()) - override def waitFor(job: JobHandle): Unit = withHandle(job)(_.job.awaitTermination()) @@ -398,11 +383,9 @@ private[sbt] class BackgroundThreadPool extends java.io.Closeable { stopListeners += result result } - override def awaitTermination(): Unit = finishedLatch.await() - - override def awaitTerminationTry(): Try[Unit] = { - awaitTermination() - exitTry.getOrElse(Try(())) + override def awaitTermination(): Unit = { + finishedLatch.await() + exitTry.foreach(_.fold(e => throw e, identity)) } override def humanReadableName: String = taskName From 7426ae520c93fe4f129ceed5cd11d4962c4425b2 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Sun, 17 Nov 2019 15:43:43 -0800 Subject: [PATCH 2/4] Fix background job shutdown When a user calls sbt exit and there is an active background job, sbt may not exit cleanly. This was primarily because the background job service shutdown method depended on the StandardMain.executionContext which was closed before the background job service was shutdown. This was fixable by reordering the resource closing in StandardMain.runManaged. --- main/src/main/scala/sbt/Main.scala | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/main/src/main/scala/sbt/Main.scala b/main/src/main/scala/sbt/Main.scala index 274c023d4..6337d1e59 100644 --- a/main/src/main/scala/sbt/Main.scala +++ b/main/src/main/scala/sbt/Main.scala @@ -126,15 +126,14 @@ object StandardMain { def runManaged(s: State): xsbti.MainResult = { val previous = TrapExit.installManager() try { + val hook = ShutdownHooks.add(closeRunnable) try { - val hook = ShutdownHooks.add(closeRunnable) - try { - MainLoop.runLogged(s) - } finally { - hook.close() - () - } - } finally DefaultBackgroundJobService.backgroundJobService.shutdown() + MainLoop.runLogged(s) + } finally { + try DefaultBackgroundJobService.backgroundJobService.shutdown() + finally hook.close() + () + } } finally TrapExit.uninstallManager(previous) } From 73edc8d4ffc31514a0d21136721fecd5d2059606 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Sat, 30 Nov 2019 15:11:26 -0800 Subject: [PATCH 3/4] Use anonymous function instead of Runnable --- .../main/scala/sbt/internal/DefaultBackgroundJobService.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala b/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala index a0328c731..7eea5304d 100644 --- a/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala +++ b/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala @@ -372,7 +372,7 @@ private[sbt] class BackgroundThreadPool extends java.io.Closeable { list } listeners.foreach { l => - l.executionContext.execute(new Runnable { override def run = l.callback() }) + l.executionContext.execute(() => l.callback()) } } } From 73a196798f0573608d4adf472606eed576a307c0 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Mon, 18 Nov 2019 11:35:04 -0800 Subject: [PATCH 4/4] Move background job service directory location Rather than putting the background job temporary files in whatever java.io.tmpdir points to, this commit moves the files into a subdirectory of target in the project root directory. To make the directory configurable via settings, I had to move the declaration of the bgJobService setting later in the project initialization process. I don't think this should matter because background jobs shouldn't be created until after the project has loaded all of its settings.. --- main/src/main/scala/sbt/Defaults.scala | 2 +- main/src/main/scala/sbt/Keys.scala | 1 + main/src/main/scala/sbt/Main.scala | 2 +- .../DefaultBackgroundJobService.scala | 53 ++++++++++++++----- main/src/main/scala/sbt/internal/Load.scala | 1 - 5 files changed, 43 insertions(+), 16 deletions(-) diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 8605a4139..688d6ca71 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -351,7 +351,7 @@ object Defaults extends BuildCommon { sys.env.contains("CI") || SysProp.ci, // watch related settings pollInterval :== Watch.defaultPollInterval, - ) ++ LintUnused.lintSettings + ) ++ LintUnused.lintSettings ++ DefaultBackgroundJobService.backgroundJobServiceSettings ) def defaultTestTasks(key: Scoped): Seq[Setting[_]] = diff --git a/main/src/main/scala/sbt/Keys.scala b/main/src/main/scala/sbt/Keys.scala index ae23c5a0d..a64ac4fec 100644 --- a/main/src/main/scala/sbt/Keys.scala +++ b/main/src/main/scala/sbt/Keys.scala @@ -251,6 +251,7 @@ object Keys { val javaOptions = taskKey[Seq[String]]("Options passed to a new JVM when forking.").withRank(BPlusTask) val envVars = taskKey[Map[String, String]]("Environment variables used when forking a new JVM").withRank(BTask) + val bgJobServiceDirectory = settingKey[File]("The directory for temporary files used by background jobs.") val bgJobService = settingKey[BackgroundJobService]("Job manager used to run background jobs.") val bgList = taskKey[Seq[JobHandle]]("List running background jobs.") val ps = taskKey[Seq[JobHandle]]("bgList variant that displays on the log.") diff --git a/main/src/main/scala/sbt/Main.scala b/main/src/main/scala/sbt/Main.scala index 6337d1e59..4da7133ff 100644 --- a/main/src/main/scala/sbt/Main.scala +++ b/main/src/main/scala/sbt/Main.scala @@ -130,7 +130,7 @@ object StandardMain { try { MainLoop.runLogged(s) } finally { - try DefaultBackgroundJobService.backgroundJobService.shutdown() + try DefaultBackgroundJobService.shutdown() finally hook.close() () } diff --git a/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala b/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala index 7eea5304d..3c8a23ded 100644 --- a/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala +++ b/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala @@ -12,7 +12,8 @@ import java.io.{ Closeable, File, FileInputStream, IOException } import java.nio.file.attribute.BasicFileAttributes import java.nio.file.{ FileVisitResult, Files, Path, SimpleFileVisitor } import java.security.{ DigestInputStream, MessageDigest } -import java.util.concurrent.atomic.AtomicLong +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.{ AtomicLong, AtomicReference } import sbt.Def.{ Classpath, ScopedKey, Setting } import sbt.Scope.GlobalScope @@ -61,13 +62,16 @@ private[sbt] abstract class AbstractBackgroundJobService extends BackgroundJobSe private val nextId = new AtomicLong(1) private val pool = new BackgroundThreadPool() - private var serviceTempDirOpt: Option[File] = None - private def serviceTempDir = serviceTempDirOpt match { - case Some(dir) => dir - case _ => - val dir = IO.createTemporaryDirectory - serviceTempDirOpt = Some(dir) - dir + private[sbt] def serviceTempDirBase: File + private val serviceTempDirRef = new AtomicReference[File] + private def serviceTempDir: File = serviceTempDirRef.synchronized { + serviceTempDirRef.get match { + case null => + val dir = IO.createUniqueDirectory(serviceTempDirBase) + serviceTempDirRef.set(dir) + dir + case s => s + } } // hooks for sending start/stop events protected def onAddJob(@deprecated("unused", "") job: JobHandle): Unit = () @@ -166,7 +170,7 @@ private[sbt] abstract class AbstractBackgroundJobService extends BackgroundJobSe } } pool.close() - serviceTempDirOpt foreach IO.delete + Option(serviceTempDirRef.get).foreach(IO.delete) } private def withHandle(job: JobHandle)(f: ThreadJobHandle => Unit): Unit = job match { @@ -465,14 +469,37 @@ private[sbt] class BackgroundThreadPool extends java.io.Closeable { } } -private[sbt] class DefaultBackgroundJobService extends AbstractBackgroundJobService { +private[sbt] class DefaultBackgroundJobService(private[sbt] val serviceTempDirBase: File) + extends AbstractBackgroundJobService { + @deprecated("Use the constructor that specifies the background job temporary directory", "1.4.0") + def this() = this(IO.createTemporaryDirectory) override def makeContext(id: Long, spawningTask: ScopedKey[_], state: State): ManagedLogger = { val extracted = Project.extract(state) LogManager.constructBackgroundLog(extracted.structure.data, state)(spawningTask) } } private[sbt] object DefaultBackgroundJobService { - lazy val backgroundJobService: DefaultBackgroundJobService = new DefaultBackgroundJobService - lazy val backgroundJobServiceSetting: Setting[_] = - ((Keys.bgJobService in GlobalScope) :== backgroundJobService) + + private[this] val backgroundJobServices = new ConcurrentHashMap[File, DefaultBackgroundJobService] + private[sbt] def shutdown(): Unit = { + backgroundJobServices.values.forEach(_.shutdown()) + backgroundJobServices.clear() + } + private[sbt] lazy val backgroundJobServiceSetting: Setting[_] = + (Keys.bgJobService in GlobalScope) := { + val path = (sbt.Keys.bgJobServiceDirectory in GlobalScope).value + val newService = new DefaultBackgroundJobService(path) + backgroundJobServices.putIfAbsent(path, newService) match { + case null => newService + case s => + newService.shutdown() + s + } + } + private[sbt] lazy val backgroundJobServiceSettings: Seq[Def.Setting[_]] = Def.settings( + Keys.bgJobServiceDirectory in GlobalScope := { + sbt.Keys.appConfiguration.value.baseDirectory / "target" / "bg-jobs" + }, + backgroundJobServiceSetting + ) } diff --git a/main/src/main/scala/sbt/internal/Load.scala b/main/src/main/scala/sbt/internal/Load.scala index 622d41cd6..dcc046081 100755 --- a/main/src/main/scala/sbt/internal/Load.scala +++ b/main/src/main/scala/sbt/internal/Load.scala @@ -130,7 +130,6 @@ private[sbt] object Load { def injectGlobal(state: State): Seq[Setting[_]] = (appConfiguration in GlobalScope :== state.configuration) +: LogManager.settingsLogger(state) +: - DefaultBackgroundJobService.backgroundJobServiceSetting +: EvaluateTask.injectSettings def defaultWithGlobal(