jboner/akka — pull request #146 by sirthias (2011-12-12 16:47:30+00:00)
Title: Fix memory leak in DefaultCompletableFuture:onTimeout

During the hunt for a memory leak in a very simple spray-client app we drilled our way all the way through to an old Java issue, which appears to be the root cause: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6213323.

The problem is that, when an onTimeout callback is attached to a DefaultCompletableFuture, this callback prevents the Future instance from being garbage collected, potentially until long after future completion. Only when the timeout callback is actually scheduled is the Future instance released. Depending on the timeout value this might create quite a big memory leak (as is the case in our sample app).

Here is the relevant snippet from DefaultCompletableFuture:onTimeout:

val runnable = new Runnable { def run() { if (!isCompleted) func(DefaultCompletableFuture.this) } }
val fut = Scheduler.scheduleOnce(runnable, timeLeft, NANOS)
onComplete(_ => fut.cancel(true))

The assumption here was that the `cancel` call in the onComplete callback cancels the scheduled timeout runnable _and frees all related references_.
However, all `cancel` does here is to mark the underlying FutureTask as "cancelled". The FutureTask remains in the executors queue and also doesn't clear the reference to its runnable, which in this case holds a reference to DefaultCompletableFuture.this.

The fix is not hard: simply introduce another layer of indirection and break the reference chain manually:

val runner = new AtomicReference(() => if (!isCompleted) func(DefaultCompletableFuture.this))
val runnable = new Runnable { def run() { val r = runner.get; if (r != null) r() } }
val fut = Scheduler.scheduleOnce(runnable, timeLeft(), NANOS)
onComplete { _ => fut.cancel(false); runner.set(null) }

(Note that I also changed the `cancel` call from `cancel(true)` to `cancel(false)`, since I don't think interrupting a potentially already running timeout callback is a good idea).