Skip to content

[1.8.x] Make Keys.loggerContext and LoggerContext() public#7147

Closed
mkurz wants to merge 2 commits into
sbt:1.8.xfrom
mkurz:public_loggerContext
Closed

[1.8.x] Make Keys.loggerContext and LoggerContext() public#7147
mkurz wants to merge 2 commits into
sbt:1.8.xfrom
mkurz:public_loggerContext

Conversation

@mkurz

@mkurz mkurz commented Feb 9, 2023

Copy link
Copy Markdown
Member

Starting with sbt 1.8.0-RC1 a LoggerContext will set its internal closed status to true:

def close(): Unit = {
closed.set(true)
loggers.forEach((_, l) => l.clearAppenders())
loggers.clear()
}

The line closed.set(true) was introduced in #6992.

Because this AtomicBoolean is true now, it of course is not possible to re use a LoggerContext anymore:

override def logger(
name: String,
channelName: Option[String],
execId: Option[String]
): ManagedLogger = {
if (closed.get) {
throw new IllegalStateException("Tried to create logger for closed LoggerContext")
}

So far so good, this seems correct.

However this is causing problems with the Play Framework, which now runs into that exception with sbt 1.8, see playframework/playframework#11527

So basically Play has a special NonBlockingInteractionMode which causes an app to continue to run in the background when calling run in a sbt console. That means, when run is "finished", there is still a (server) thread running that can serve requests. We use that for scripted tests. It's actually the same mechanism that sbt's bgRun is using, only that NonBlockingInteractionMode was implemented long time before bgRun was introduced in sbt 1.0.
Until sbt 1.7.x this was working fine, even though the background thread was using a state's loggercontext that, in theory, was closed already, but wasn't marked as closed (even thought the close method ran).

To test things out, I also came up with a clean room bgRun implementation in Play, that resembled what the NonBlockingInteractionMode is doing and I was running in the exact same exception. That bgRun looked somehow like this:

// Not the full code, just for demonstration purposes
bgRun: Initialize[InputTask[JobHandle]] = Def.inputTask {
    val service = bgJobService.value
    service.runInBackground(resolvedScoped.value, state.value) { (logger, workingDir) =>
      // At some point later, triggered by the Play server when a request comes in and the app has code changes:
      Project.runTask(Compile / compile, state) // re-compile the project
    }
}.evaluated

The problem is that the body of runInBackground, which at some point may calls Project.runTask, is not running in the bgRun lifetime anymore, but in a background thread, so after the bgRun task finishes the loggercontext gets closed. Project.runTask(...) however tries to access the loggercontext of the passed state which of course is closed now.

I tried a lot to workaround that or to come up with another solution. However the only solution that I found and IMHO think is the only one to be able to resolve that (right now) is to "wrap" Project.runTask(...) calls with an open loggercontext, which needs to be attached to the state. To do that, I need to be able to

  1. initialized a LoggerContext - but the apply method is private right now
  2. attach that loggerContext to the state with state.put(Keys.loggerContext,..). - but Keys.loggerContext is private right now

Using this patch here I was able to make both, Play's NonBlockingInteractionMode and sbt's bgRun, work in Play.
These lines of playframework/playframework#11441 create a loggercontext and closes it afterwards. I also finally was able to implement bgRun in Play in the last commit of that PR.

@eed3si9n If there is nothing that speaks against opening up this key and method I would be happy if you can merge this, I will provide PRs for the 1.9.x and develop branch afterwards.

@mkurz mkurz force-pushed the public_loggerContext branch from f2896ae to 2c65325 Compare February 9, 2023 16:19
}
def close(): Unit = {
closed.set(true)
def close(): Unit = if (closed.compareAndSet(false, true)) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small enhancement to make sure close() runs only once, like in line 110 of this file.

@eed3si9n

eed3si9n commented Feb 9, 2023

Copy link
Copy Markdown
Member

One workaround might be moving PlayRun into sbt.play.run package, and define an alias in the play.sbt.run package object to compatibility.

@mkurz

mkurz commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

Yeah, I was thinking about that. However, I am just wondering is there a reason to keep those things private?

@eed3si9n

eed3si9n commented Feb 9, 2023

Copy link
Copy Markdown
Member

Yeah, I was thinking about that. However, I am just wondering is there a reason to keep those things private?

private[sbt] and sbt.internal are our signal that the implementation is subject to change, and not covered by binary compatibility. The less we can expose the sbt internals out to the plugin ecosystem the better for general stability.

@mkurz

mkurz commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

@eed3si9n Alright, I moved the object to the sbt package now, so let's close that PR for now.

@mkurz mkurz closed this Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants