Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a LinkedBlockingQueue instead of polling. #44

Merged
merged 1 commit into from
May 15, 2024
Merged

Use a LinkedBlockingQueue instead of polling. #44

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions modules/framework/jvm/src/main/scala/RunnerCompat.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package framework
import org.typelevel.scalaccompat.annotation.unused

import java.io.PrintStream
import java.util.concurrent.ConcurrentLinkedQueue
import java.util.concurrent.atomic.{ AtomicBoolean, AtomicInteger }

import scala.concurrent.duration._
Expand All @@ -16,6 +15,7 @@ import cats.effect.{ Ref, Sync, _ }
import cats.syntax.all._

import sbt.testing.{ Task, TaskDef }
import java.util.concurrent.LinkedBlockingQueue

trait RunnerCompat[F[_]] { self: sbt.testing.Runner =>

Expand Down Expand Up @@ -73,7 +73,7 @@ trait RunnerCompat[F[_]] { self: sbt.testing.Runner =>
// dispatching logs through a single logger at a time.
val loggerPermit = new java.util.concurrent.Semaphore(1, true)

val queue = new ConcurrentLinkedQueue[SuiteEvent]()
val queue = new LinkedBlockingQueue[SuiteEvent]()
val broker = new ConcurrentQueueEventBroker(queue)
val startingBlock = unsafeRun.fromFuture {
promise.future.map(_ => ())(ExecutionContext.global)
Expand Down Expand Up @@ -252,7 +252,7 @@ trait RunnerCompat[F[_]] { self: sbt.testing.Runner =>
}

class ConcurrentQueueEventBroker(
concurrentQueue: ConcurrentLinkedQueue[SuiteEvent])
concurrentQueue: LinkedBlockingQueue[SuiteEvent])
extends SuiteEventBroker {
def send(suiteEvent: SuiteEvent): F[Unit] = {
Sync[F].delay(concurrentQueue.add(suiteEvent)).void
Expand Down
5 changes: 3 additions & 2 deletions modules/framework/jvm/src/main/scala/SbtTask.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@ import java.util.concurrent.atomic.{ AtomicBoolean, AtomicInteger }
import cats.data.Chain

import sbt.testing.{ Event, EventHandler, Logger, Task, TaskDef }
import java.util.concurrent.TimeUnit

private[framework] class SbtTask(
val taskDef: TaskDef,
isDone: AtomicBoolean,
stillRunning: AtomicInteger,
waitForResourcesShutdown: java.util.concurrent.Semaphore,
start: scala.concurrent.Promise[Unit],
queue: java.util.concurrent.ConcurrentLinkedQueue[SuiteEvent],
queue: java.util.concurrent.LinkedBlockingQueue[SuiteEvent],
loggerPermit: java.util.concurrent.Semaphore,
readFailed: () => Chain[(SuiteName, TestOutcome)]
) extends Task {
Expand All @@ -30,7 +31,7 @@ private[framework] class SbtTask(
loggerPermit.acquire()
try {
while (!finished && !isDone.get()) {
val nextEvent = Option(queue.poll())
val nextEvent = Option(queue.poll(1, TimeUnit.SECONDS))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 1 second time is arbitrary.

The loop should terminate when isDone is set, so probably shouldn't poll indefinitely. From reading the code, isDone ought to be set whenever there's an error creating global resources or whenever SBT calls done on the runner. I haven't been able to simulate either of these cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think 1 second is fine.

It's really hard to test a test framework via the SBT interface, so as long as the build still builds, I think your changes are fine 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested with scala-cli and SBT, and it seems to work fine.

You can reproduce the spinlock with

scala-cli test https://gist.github.com/zainab-ali/867fbc095d2ab3ceda498b29bc2b8aba

Here's a screenshot of the CPU usage before:
before

And after (with the current changes):
after

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome, that's sufficient to get it merged then 👍


nextEvent.foreach {
case s @ SuiteStarted(_) => log(s)
Expand Down