Skip to content

Make timeouts actually cancel the underlying processes #133

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.

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

Merged
merged 6 commits into from
Dec 7, 2021

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
@RicoGit RicoGit 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