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

Already on GitHub? Sign in to your account

DefaultClient doesn't close bound dest Var #240

Closed
gianm opened this Issue Feb 7, 2014 · 6 comments

Comments

3 participants

gianm commented Feb 7, 2014

DefaultClient.scala calls dest.bind(), observes it, and discards the resulting Closable. This prevents cleanup actions from occurring on the underlying Var.

Contributor

bmdhacks commented Feb 7, 2014

Wouldn't the whole bundle get cleaned up when the DefaultClient is non-reachable from gc roots?

gianm commented Feb 7, 2014

I don't think there are finalizers in place that close stuff, so I don't think so. Even if they exist, though, I'd think that Closables on the dest Var[Addr] should get called when the Service is closed, instead of having to wait for finalization.

Contributor

bmdhacks commented Feb 8, 2014

I don't think you need to finalize them because the close trait is just an interface, we don't actually have open OS handles that really need closing. But hey, if you have a heap dump that proves me wrong, that's cool. I'm pretty sure though that the whole nest of Name+DefaultClient will go away when the client is closed/gc'd

gianm commented Feb 8, 2014

My problem is that I'm using Var.async to create Vars that encapsulate external resources and need cleaning up. The "update" method starts a Curator PathChildrenCache and returns a Closable that closes the cache. What I see is that even when the client Service is closed, the Closable never gets called and so the cache never stops and the Curator structures don't get cleaned up. (In my case the Curator client belongs to the Resolver, not the Var, so it persists for the life of the program even though the Vars often don't.)

Code is something like:

import com.twitter.finagle.builder.ClientBuilder
import com.twitter.finagle.http.Http
import com.twitter.finagle.{Addr, Resolver}
import com.twitter.util.{Await, Future, Time, Closable, Var}
import java.net.InetSocketAddress

object hey
{
  def main(args: Array[String]) {
    val client = ClientBuilder().codec(Http()).hostConnectionLimit(1).dest("hey!what").build()
    Await.ready(client.close())
  }
}

class HeyResolver extends Resolver
{
  override val scheme = "hey"

  override def bind(arg: String) = Var.async[Addr](Addr.Bound(new InetSocketAddress("example.com", 80))) {
    updatable =>
      println("Observing!")
      new Closable {
        override def close(deadline: Time) = {
          println("Closing!")
          Future.Done
        }
      }
  }
}

So I'd expect "Closing!" to happen when the Service is closed, but it doesn't. I also don't think a GC would help-- PathChildrenCaches need to be closed and not just thrown away.

Contributor

bmdhacks commented Feb 10, 2014

https://gist.github.com/bmdhacks/3100043c69478b9f7c57

This should fix it. I'm working on getting this into our internal repo and it should be published shortly.

Contributor

mosesn commented Apr 21, 2014

This got merged, thanks for the bug report!

@mosesn mosesn closed this Apr 21, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment