twitter / finagle

A fault tolerant, protocol-agnostic RPC system
https://twitter.github.io/finagle
Apache License 2.0
8.79k stars 1.46k forks source link

Unable to stop CollectClosables thread #758

Open dani-e-l opened 5 years ago

dani-e-l commented 5 years ago

Hello we are using Finagle as a HttpClient - it works really good. Great job.

Unfortunately Finagle generates resource leak under standard behavior that applications are deployed and undeployed many times on JBoss AS, Tomcat etc. By resource leak I mean that if I 100 times deploy and undeploy an application that uses Finagle I'll get at least 100 threads

The problem is that finagle contains unstopable threads

Var.scala

  def flatMap[U](f: T => Var[U]): Var[U] = new Var[U] {
    def observe(depth: Int, obs: Observer[U]) = {
      val inner = new AtomicReference(Closable.nop)

and this starts

  private val collectorThread = new Thread("CollectClosables") {
    override def run(): Unit = {
      while (true) {
        try {
kevinoliver commented 5 years ago

Interesting. Would an API for shutting it down suffice? It may need some reworking along with the API...

dani-e-l commented 5 years ago

Thanks for quick reply Kevin. I was able to close many other threads because API allows me to use my own Timers/Executors/etc I have problem with last three threads.

In this case Finagle uses Closable.nop but this fires CollectClosables.collectorThread field initialization that starts Timer. Unfortunately this field is private and Thread uninterruptible.

Yes API is good. I'm not sure if is is good idea to use while (true) and catch inside InterruptedException. I suggest to use while(flag)

rgala commented 5 years ago

In Closeable.scala, you could consider calling the remove() method with some parameterized timeout:

val ref = refq.remove(...)

Currently we use Finagle in a Spring Boot app deployed on extrernal Tomcat. The CollectClosables thread prevents Tomcat from shutting down gracefully because it waits for this thread to terminate, which never happens.

rgala commented 5 years ago

I tried to debug the com.twitter.util.Closable, the following method is never executed and this is probably why the remove method blocks forever:

  def closeOnCollect(closable: Closable, obj: Object): Unit = refs.synchronized {
    refs.put(new PhantomReference(obj, refq), closable)
  }
mosesn commented 5 years ago

@rgala can you configure your Spring Boot app to not wait for daemon threads to terminate before shutting down? I think that's a standard JVM behavior. Or when you say "shutdown" do you mean that you're not actually shutting down the VM but that you're only unloading classes?

I'm not familiar with JBoss AS or Tomcat, we typically assume that scala objects will only be initialized once. Does it unload the classes then load them again?

rgala commented 5 years ago

By "shutdown" I mean telling Tomcat to stop. As a result, Tomcat first undeploys all deployed applications and waits for all threads they submitted to terminate. Since the CollectClosables never terminates, Tomcat fails to stop.

rgala commented 5 years ago

One more thing. This happens only when Tomcat is run as a Windows service using Apache Commons Daemon (https://commons.apache.org/proper/commons-daemon/)

mosesn commented 5 years ago

OK, I would be happy with accepting a pull request to add a stopCollectClosablesThread method to Closable. I would also be happy with the CollectClosables thread checking the interrupt status and shutting down if it's interrupted. Whichever the implementor prefers.