whisklabs / docker-it-scala

Docker integration testing kit with Scala
MIT License
431 stars 91 forks source link

Make timeouts actually cancel the underlying processes #133

Closed pfcoperez closed 2 years ago

pfcoperez commented 5 years ago

Currently, PullImagesTimeout, StartContainersTimeout, and StopContainersTimeout timeouts are enforced by Await.ready, Await.result, ... e.g:

https://github.com/whisklabs/docker-it-scala/blob/4d3bc7e8630fcdb7bc1a51a11a49f543564b38a4/core/src/main/scala/com/whisk/docker/DockerKit.scala#L57-L60

When the timeout expires, however, the Future's body code keeps running even if the TimeoutException has been raised. As an example, run this code in a Scala REPL:

Await.ready(Future { Thread.sleep(5000); println("BOOM!")}, 1 second)

The result is that the exception is raised and yet "BOOM" is printed:

java.util.concurrent.TimeoutException: Futures timed out after [1 second]
  scala.concurrent.impl.Promise$DefaultPromise.ready(Promise.scala:223)
  scala.concurrent.impl.Promise$DefaultPromise.ready(Promise.scala:157)
  scala.concurrent.Await$$anonfun$ready$1.apply(package.scala:169)
  scala.concurrent.Await$$anonfun$ready$1.apply(package.scala:169)
  scala.concurrent.BlockContext$DefaultBlockContext$.blockOn(BlockContext.scala:53)
  scala.concurrent.Await$.ready(package.scala:169)
  ammonite.$sess.cmd65$.<init>(cmd65.sc:1)
  ammonite.$sess.cmd65$.<clinit>(cmd65.sc)

@ BOOM!

This means that, as we try to stop the containers in afterAll:

https://github.com/whisklabs/docker-it-scala/blob/4d3bc7e8630fcdb7bc1a51a11a49f543564b38a4/scalatest/src/main/scala/com/whisk/docker/scalatest/DockerTestKit.scala#L24-L28

We might still be trying to start them in a separate thread.

This PR introduces timeout parameters in DockerCommandExecutor methods which propagates down to the implementations of this interface. There, it replaces Future.apply factory invocation with PerishableFuture.apply. It also adds PerishableFuture factory which makes sure the body gets interrupted when the timeout expires:

case finiteTimeout: FiniteDuration =>
        val promise = Promise[T]

        val futureTask = new FutureTask[T](new Callable[T] {
          override def call(): T = body
        }) {
          override def done(): Unit = promise.tryComplete {
            Try(get()).recoverWith {
              case _: CancellationException => Failure(new TimeoutException())
            }
          }
        }

        val reaperTask = new TimerTask {
          override def run(): Unit = {
            futureTask.cancel(true)
            promise.tryFailure(new TimeoutException())
          }
        }

        timer.schedule(reaperTask, finiteTimeout.toMillis)
        ec.execute(futureTask)

        promise.future

If the passed timeout is Duration.Inf it just doesn't timeout so it uses standard Future factory:

case _ => Future.apply(body)

These changes should improve host resources usage and pave the way to more advanced timeouts customization.