Skip to content

Conversation

@pfcoperez
Copy link
Contributor

@pfcoperez pfcoperez commented Sep 30, 2019

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

if (!allRunning) {
Await.ready(containerManager.stopRmAll(), StopContainersTimeout)
throw new RuntimeException("Cannot run all required containers")
}

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:

override def afterAll(): Unit = {
stopAllQuietly()
super.afterAll()
}

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.

pfcoperez and others added 6 commits September 30, 2019 16:20
…. Use that timeout in the implementers of the interface to timeout docker operations when the passed timeout is `FiniteDuration`.
…ainersTimeout` down `DockerContainerManager` and, therefore, down to `DockerCommandExecutor` operations which will use `PerishableFuture` to make ops actually cancel when they timeout.
…erations_timeouts_cancelexec

# Conflicts:
#	core/src/main/scala/com/whisk/docker/DockerContainerState.scala
#	scalatest/src/test/scala/com/whisk/docker/DockerContainerLinkingSpec.scala
#	scalatest/src/test/scala/com/whisk/docker/ElasticsearchServiceSpec.scala
#	scalatest/src/test/scala/com/whisk/docker/MongodbServiceSpec.scala
@c-solo c-solo merged commit 2302b0d into whisklabs:master Dec 7, 2021
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