From 6bcda6684add413956bc0d28bfb6b94a64f7feeb Mon Sep 17 00:00:00 2001 From: Adrien Piquerez Date: Thu, 4 Aug 2022 10:54:43 +0200 Subject: [PATCH 1/3] Use BackgroundJobService context instead of MainLoop context A new context is created and closed for each state of the MainLoop. But the context of the backgroundJob must stay alive. So we use a context that is owned by the BackgroundJobService. It creates a new logger for each background job and cleans it when the job stops. --- .../src/main/scala/sbt/util/LoggerContext.scala | 3 ++- .../internal/DefaultBackgroundJobService.scala | 15 ++++----------- main/src/main/scala/sbt/internal/LogManager.scala | 12 ++++++++++-- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/internal/util-logging/src/main/scala/sbt/util/LoggerContext.scala b/internal/util-logging/src/main/scala/sbt/util/LoggerContext.scala index ee1069061..0435793dd 100644 --- a/internal/util-logging/src/main/scala/sbt/util/LoggerContext.scala +++ b/internal/util-logging/src/main/scala/sbt/util/LoggerContext.scala @@ -182,7 +182,8 @@ object LoggerContext { } } def close(): Unit = { - loggers.forEach((name, l) => l.clearAppenders()) + closed.set(true) + loggers.forEach((_, l) => l.clearAppenders()) loggers.clear() } } diff --git a/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala b/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala index b9effa8a9..79d431ebd 100644 --- a/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala +++ b/main/src/main/scala/sbt/internal/DefaultBackgroundJobService.scala @@ -74,6 +74,7 @@ private[sbt] abstract class AbstractJobHandle extends JobHandle { private[sbt] abstract class AbstractBackgroundJobService extends BackgroundJobService { private val nextId = new AtomicLong(1) private val pool = new BackgroundThreadPool() + private val context = LoggerContext(useLog4J) private[sbt] def serviceTempDirBase: File private[sbt] def useLog4J: Boolean @@ -90,7 +91,6 @@ private[sbt] abstract class AbstractBackgroundJobService extends BackgroundJobSe // hooks for sending start/stop events protected def onAddJob(@deprecated("unused", "") job: JobHandle): Unit = () protected def onRemoveJob(@deprecated("unused", "") job: JobHandle): Unit = () - private val context = LoggerContext(useLog4J) // this mutable state could conceptually go on State except // that then every task that runs a background job would have @@ -122,12 +122,9 @@ private[sbt] abstract class AbstractBackgroundJobService extends BackgroundJobSe def humanReadableName: String = job.humanReadableName job.onStop { () => - // TODO: Fix this - // logger.close() removeJob(this) IO.delete(workingDirectory) context.clearAppenders(logger.name) - context.close() } addJob(this) override final def equals(other: Any): Boolean = other match { @@ -144,15 +141,15 @@ private[sbt] abstract class AbstractBackgroundJobService extends BackgroundJobSe override val spawningTask: ScopedKey[_] = unknownTask } - protected def makeContext(id: Long, spawningTask: ScopedKey[_], state: State): ManagedLogger - def doRunInBackground( spawningTask: ScopedKey[_], state: State, start: (Logger, File) => BackgroundJob ): JobHandle = { val id = nextId.getAndIncrement() - val logger = makeContext(id, spawningTask, state) + val extracted = Project.extract(state) + val logger = + LogManager.constructBackgroundLog(extracted.structure.data, state, context)(spawningTask) val workingDir = serviceTempDir / s"job-$id" IO.createDirectory(workingDir) val job = try { @@ -502,10 +499,6 @@ private[sbt] class DefaultBackgroundJobService( ) extends AbstractBackgroundJobService { @deprecated("Use the constructor that specifies the background job temporary directory", "1.4.0") def this() = this(IO.createTemporaryDirectory, false) - 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 { diff --git a/main/src/main/scala/sbt/internal/LogManager.scala b/main/src/main/scala/sbt/internal/LogManager.scala index e28fd91ee..7029cde2e 100644 --- a/main/src/main/scala/sbt/internal/LogManager.scala +++ b/main/src/main/scala/sbt/internal/LogManager.scala @@ -73,15 +73,23 @@ object LogManager { manager(data, state, task, to, context) } - @nowarn + @deprecated("Use alternate constructBackgroundLog that provides a LoggerContext", "1.8.0") def constructBackgroundLog( data: Settings[Scope], state: State + ): ScopedKey[_] => ManagedLogger = { + val context = state.get(Keys.loggerContext).getOrElse(LoggerContext.globalContext) + constructBackgroundLog(data, state, context) + } + + def constructBackgroundLog( + data: Settings[Scope], + state: State, + context: LoggerContext ): (ScopedKey[_]) => ManagedLogger = (task: ScopedKey[_]) => { val manager: LogManager = (logManager in task.scope).get(data) getOrElse defaultManager(state.globalLogging.console) - val context = state.get(Keys.loggerContext).getOrElse(LoggerContext.globalContext) manager.backgroundLog(data, state, task, context) } From 592086b889fa998abc436139c3ffb52153049db9 Mon Sep 17 00:00:00 2001 From: Adrien Piquerez Date: Thu, 4 Aug 2022 15:59:25 +0200 Subject: [PATCH 2/3] Don't use ProxyTerminal in background job If we use the ProxyTerminal in the background jobs, the logs would be spread across different terminals, switching from active client to active client. We want the logs to stick to the client that started the job. --- .../src/main/scala/sbt/internal/util/Terminal.scala | 1 + main/src/main/scala/sbt/internal/LogManager.scala | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/util-logging/src/main/scala/sbt/internal/util/Terminal.scala b/internal/util-logging/src/main/scala/sbt/internal/util/Terminal.scala index 0019b04e9..27b1e29c4 100644 --- a/internal/util-logging/src/main/scala/sbt/internal/util/Terminal.scala +++ b/internal/util-logging/src/main/scala/sbt/internal/util/Terminal.scala @@ -438,6 +438,7 @@ object Terminal { override def toString: String = s"ProxyTerminal(current = $t)" } private[sbt] def get: Terminal = ProxyTerminal + private[sbt] def current: Terminal = activeTerminal.get private[sbt] def withIn[T](in: InputStream)(f: => T): T = { val original = inputStream.get diff --git a/main/src/main/scala/sbt/internal/LogManager.scala b/main/src/main/scala/sbt/internal/LogManager.scala index 7029cde2e..8612c692b 100644 --- a/main/src/main/scala/sbt/internal/LogManager.scala +++ b/main/src/main/scala/sbt/internal/LogManager.scala @@ -149,7 +149,7 @@ object LogManager { task: ScopedKey[_], context: LoggerContext ): ManagedLogger = { - val console = screen(task, state) + val console = ConsoleAppender("bg-" + ConsoleAppender.generateName(), ITerminal.current) LogManager.backgroundLog(data, state, task, console, relay(()), context) } } From 634e8799e76395f7bb280ea7f5f910f9cce21838 Mon Sep 17 00:00:00 2001 From: Adrien Piquerez Date: Fri, 5 Aug 2022 14:45:38 +0200 Subject: [PATCH 3/3] Catch ClosedChannelException in background job logger We want the background job to stay alive even if its terminal has been closed and we cannot write to it anymore --- .../sbt/internal/util/ConsoleAppender.scala | 23 +++++++++++++++---- .../scala/sbt/internal/util/ConsoleOut.scala | 21 +++++++++++++++++ .../main/scala/sbt/internal/LogManager.scala | 2 +- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/internal/util-logging/src/main/scala/sbt/internal/util/ConsoleAppender.scala b/internal/util-logging/src/main/scala/sbt/internal/util/ConsoleAppender.scala index 7cfed50e6..2b1700572 100644 --- a/internal/util-logging/src/main/scala/sbt/internal/util/ConsoleAppender.scala +++ b/internal/util-logging/src/main/scala/sbt/internal/util/ConsoleAppender.scala @@ -126,11 +126,12 @@ object ConsoleAppender { def out: ConsoleOut } private[sbt] object Properties { - def from(terminal: Terminal): Properties = new Properties { - override def isAnsiSupported: Boolean = terminal.isAnsiSupported - override def isColorEnabled: Boolean = terminal.isColorEnabled - override def out = ConsoleOut.terminalOut(terminal) - } + def from(terminal: Terminal): Properties = + from(ConsoleOut.terminalOut(terminal), terminal.isAnsiSupported, terminal.isColorEnabled) + + def safelyFrom(terminal: Terminal): Properties = + from(ConsoleOut.safeTerminalOut(terminal), terminal.isAnsiSupported, terminal.isColorEnabled) + def from(o: ConsoleOut, ansi: Boolean, color: Boolean): Properties = new Properties { override def isAnsiSupported: Boolean = ansi override def isColorEnabled: Boolean = color @@ -246,6 +247,18 @@ object ConsoleAppender { new ConsoleAppender(name, Properties.from(terminal), noSuppressedMessage) } + /** + * A new `ConsoleAppender` identified by `name`, and that writes to `terminal`. + * Printing to this Appender will not throw if the Terminal has been closed. + * + * @param name An identifier for the `ConsoleAppender`. + * @param terminal The terminal to which this appender corresponds + * @return A new `ConsoleAppender` that writes to `terminal`. + */ + def safe(name: String, terminal: Terminal): Appender = { + new ConsoleAppender(name, Properties.safelyFrom(terminal), noSuppressedMessage) + } + /** * A new `ConsoleAppender` identified by `name`, and that writes to `out`. * diff --git a/internal/util-logging/src/main/scala/sbt/internal/util/ConsoleOut.scala b/internal/util-logging/src/main/scala/sbt/internal/util/ConsoleOut.scala index b946a0cc8..0f6a089bf 100644 --- a/internal/util-logging/src/main/scala/sbt/internal/util/ConsoleOut.scala +++ b/internal/util-logging/src/main/scala/sbt/internal/util/ConsoleOut.scala @@ -8,6 +8,7 @@ package sbt.internal.util import java.io.{ BufferedWriter, PrintStream, PrintWriter } +import java.nio.channels.ClosedChannelException import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.atomic.AtomicReference @@ -90,6 +91,26 @@ object ConsoleOut { override def toString: String = s"TerminalOut" } + /** Same as terminalOut but it catches and ignores the ClosedChannelException + */ + def safeTerminalOut(terminal: Terminal): ConsoleOut = { + val out = terminalOut(terminal) + new ConsoleOut { + override val lockObject: AnyRef = terminal + override def print(s: String): Unit = catchException(out.print(s)) + override def println(s: String): Unit = catchException(out.println(s)) + override def println(): Unit = catchException(out.println()) + override def flush(): Unit = catchException(out.flush) + override def toString: String = s"SafeTerminalOut($terminal)" + private def catchException(f: => Unit): Unit = { + try f + catch { + case _: ClosedChannelException => () + } + } + } + } + private[this] val consoleOutPerTerminal = new ConcurrentHashMap[Terminal, ConsoleOut] def terminalOut(terminal: Terminal): ConsoleOut = consoleOutPerTerminal.get(terminal) match { case null => diff --git a/main/src/main/scala/sbt/internal/LogManager.scala b/main/src/main/scala/sbt/internal/LogManager.scala index 8612c692b..9feb065d1 100644 --- a/main/src/main/scala/sbt/internal/LogManager.scala +++ b/main/src/main/scala/sbt/internal/LogManager.scala @@ -149,7 +149,7 @@ object LogManager { task: ScopedKey[_], context: LoggerContext ): ManagedLogger = { - val console = ConsoleAppender("bg-" + ConsoleAppender.generateName(), ITerminal.current) + val console = ConsoleAppender.safe("bg-" + ConsoleAppender.generateName(), ITerminal.current) LogManager.backgroundLog(data, state, task, console, relay(()), context) } }