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 HighResTimer #756

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

Problem is withh HighResTimer Thread There is no possibility to stop it using Finagle API.

object HighResTimer {

  /**
   * The default Timer used for configuration.
   *
   * It is a shared resource and as such, `stop` is ignored.
   */
  val Default: com.twitter.util.Timer =
    new JavaTimer(true, Some("HighResTimer")) {
      override def stop(): Unit = ()
    }

Unfortunately we can't use our own HighResTimer using params because Default is initialized when Http.client is created and there are two instances

 /**
   * Retries failures that are guaranteed to be safe to retry
   * (see [[RetryPolicy.RetryableWriteException]]).
   */
  private[finagle] def moduleRequeueable[Req, Rep]: Stackable[ServiceFactory[Req, Rep]] =
    new Stack.Module[ServiceFactory[Req, Rep]] {
      def role: Stack.Role = Retries.Role

      def description: String =
        "Retries requests, at the service application level, that have been rejected"

      val parameters = Seq(
        implicitly[Stack.Param[Stats]],
        implicitly[Stack.Param[Budget]],
        implicitly[Stack.Param[HighResTimer]]
      )

There are two alternative solutions

  1. possibility to stop this Thread
  2. Do not instantiate HighResTimer when Client is created
kevinoliver commented 5 years ago

Ah, yep another instance of this. Just like #757 .

I think same sort of solution noted there could work here as well.

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 HighResTimer param is injected implicitly when client is instantiated. It is definitely to early. In such implementation StackParam[HighResTimer] makes no sense