finagle-core: Make ServiceFactory#status abstract

Problem

ServiceFactory#status being by default Open has been error-prone to let the
wrapping service not closable.

Solution

Make the method abstract and always requires an implementation.

JIRA Issues: CSL-12036

Differential Revision: https://phabricator.twitter.biz/D933288

Author: yufangong

CHANGELOG.rst

@@ -40,6 +40,9 @@ Breaking API Changes
 
 * finagle-core: `Trace.recordLocalSpan` is `private[this]` and no longer `protected`. ``PHAB_ID=D912760``
 
+* finagle-core: "ServiceFactory#status" is abstract and requires implementation in the inherited
+  classes. ``PHAB_ID=D933288``
+
 Runtime Behavior Changes
 ~~~~~~~~~~~~~~~~~~~~~~~~
 

finagle-benchmark/src/main/scala/com/twitter/finagle/loadbalancer/BalancerBench.scala

@@ -28,6 +28,8 @@ object BalancerBench {
           def apply(req: Unit): Future[Unit] = Future.Done
         })
       def close(when: Time): Future[Unit] = Future.Done
+
+      def status: Status = Status.Open
     }
 
   def newActivity(num: Int): Activity[Vector[EndpointFactory[Unit, Unit]]] = {

finagle-benchmark/src/main/scala/com/twitter/finagle/loadbalancer/ServerFactory.scala

@@ -1,9 +1,15 @@
 package com.twitter.finagle.loadbalancer
 
-import com.twitter.finagle.{Address, ClientConnection, Service, Failure}
+import com.twitter.finagle.Address
+import com.twitter.finagle.ClientConnection
+import com.twitter.finagle.Service
+import com.twitter.finagle.Failure
+import com.twitter.finagle.Status
 import com.twitter.finagle.stats.StatsReceiver
 import com.twitter.finagle.util.DefaultTimer
-import com.twitter.util.{Duration, Future, Time}
+import com.twitter.util.Duration
+import com.twitter.util.Future
+import com.twitter.util.Time
 import java.util.concurrent.atomic.AtomicInteger
 
 private trait Server extends EndpointFactory[Unit, Unit] {
@@ -81,6 +87,7 @@ private object ServerFactory {
     def count: Long = _numRequests.get().toLong
     def apply(conn: ClientConnection): Future[Service[Unit, Unit]] = Future.value(service)
     def close(deadline: Time): Future[Unit] = Future.Done
+    def status: Status = service.status
     override def toString: String = id
   }
 }

finagle-core/src/main/scala/com/twitter/finagle/ServiceFactory.scala

@@ -1,6 +1,10 @@
 package com.twitter.finagle
 
-import com.twitter.util.{Closable, Future, Return, Throw, Time}
+import com.twitter.util.Closable
+import com.twitter.util.Future
+import com.twitter.util.Return
+import com.twitter.util.Throw
+import com.twitter.util.Time
 
 abstract class ServiceFactory[-Req, +Rep]
     extends (ClientConnection => Future[Service[Req, Rep]])
@@ -66,7 +70,7 @@ abstract class ServiceFactory[-Req, +Rep]
   /**
    * The current availability [[Status]] of this ServiceFactory
    */
-  def status: Status = Status.Open
+  def status: Status
 
   /**
    * Return `true` if and only if [[status]] is currently [[Status.Open]].
@@ -104,6 +108,7 @@ object ServiceFactory {
     new ServiceFactory[Req, Rep] {
       def apply(_conn: ClientConnection): Future[Service[Req, Rep]] = f()
       def close(deadline: Time): Future[Unit] = Future.Done
+      def status: Status = Status.Open
 
       override def toString: String = s"${getClass.getName}(${f.toString()})"
     }

finagle-core/src/main/scala/com/twitter/finagle/pool/BalancingPool.scala

@@ -101,6 +101,7 @@ private[finagle] object BalancingPool {
       params)
 
     def apply(conn: ClientConnection): Future[Service[Req, Rep]] = balancer()
+    def status: Status = balancer.status
 
     def close(deadline: Time): Future[Unit] =
       balancer

finagle-core/src/test/scala/com/twitter/finagle/FilterTest.scala

@@ -1,11 +1,19 @@
 package com.twitter.finagle
 
 import com.twitter.conversions.DurationOps._
-import com.twitter.finagle.service.{ConstantService, NilService}
-import com.twitter.util.{Await, Future, Promise, Time}
-import java.util.concurrent.atomic.{AtomicBoolean, AtomicInteger}
+import com.twitter.finagle.service.ConstantService
+import com.twitter.finagle.service.NilService
+import com.twitter.util.Await
+import com.twitter.util.Future
+import com.twitter.util.Promise
+import com.twitter.util.Time
+import java.util.concurrent.atomic.AtomicBoolean
+import java.util.concurrent.atomic.AtomicInteger
 import org.mockito.Matchers._
-import org.mockito.Mockito.{never, spy, times, verify}
+import org.mockito.Mockito.never
+import org.mockito.Mockito.spy
+import org.mockito.Mockito.times
+import org.mockito.Mockito.verify
 import org.scalatest.funsuite.AnyFunSuite
 
 class FilterTest extends AnyFunSuite {
@@ -24,6 +32,7 @@ class FilterTest extends AnyFunSuite {
   class PassThruServiceFactory extends ServiceFactory[Int, Int] {
     def apply(conn: ClientConnection): Future[Service[Int, Int]] = Future.value(constSvc)
     def close(deadline: Time): Future[Unit] = Future.Done
+    def status: Status = Status.Open
   }
 
   val constSvc = new ConstantService[Int, Int](Future.value(2))

finagle-core/src/test/scala/com/twitter/finagle/ResolverTest.scala

@@ -1,13 +1,16 @@
 package com.twitter.finagle
 
-import com.twitter.util.{Future, Time, Var}
+import com.twitter.util.Future
+import com.twitter.util.Time
+import com.twitter.util.Var
 import org.scalatest.funsuite.AnyFunSuite
 
 object TestAddr {
   case class StringFactory(s: String) extends ServiceFactory[Any, String] {
     val svc = Service.const(Future.value(s))
     override def apply(conn: ClientConnection) = Future.value(svc)
     override def close(deadline: Time) = Future.Done
+    def status: Status = svc.status
   }
 
   def apply(arg: String): Address = {

finagle-core/src/test/scala/com/twitter/finagle/ServiceFactoryTest.scala

@@ -1,7 +1,9 @@
 package com.twitter.finagle
 
-import com.twitter.finagle.service.{ConstantService, ServiceFactoryRef}
-import com.twitter.util.{Future, Time}
+import com.twitter.finagle.service.ConstantService
+import com.twitter.finagle.service.ServiceFactoryRef
+import com.twitter.util.Future
+import com.twitter.util.Time
 import org.scalatest.funsuite.AnyFunSuite
 
 class ServiceFactoryTest extends AnyFunSuite {
@@ -11,6 +13,8 @@ class ServiceFactoryTest extends AnyFunSuite {
       Future.value(new ConstantService[Int, Int](Future.value(7)))
 
     def close(deadline: Time): Future[Unit] = Future.Done
+
+    def status: Status = Status.Open
   }
 
   case class TestProxy(_self: ServiceFactory[Int, Int]) extends ServiceFactoryProxy(_self)

finagle-core/src/test/scala/com/twitter/finagle/ServiceTest.scala

@@ -1,10 +1,14 @@
 package com.twitter.finagle
 
 import com.twitter.conversions.DurationOps._
-import com.twitter.finagle.service.{ConstantService, FailedService, NilService}
+import com.twitter.finagle.service.ConstantService
+import com.twitter.finagle.service.FailedService
+import com.twitter.finagle.service.NilService
 import com.twitter.util._
 import org.mockito.Matchers._
-import org.mockito.Mockito.{times, verify, when}
+import org.mockito.Mockito.times
+import org.mockito.Mockito.verify
+import org.mockito.Mockito.when
 import org.mockito.invocation.InvocationOnMock
 import org.mockito.stubbing.Answer
 import org.scalatestplus.mockito.MockitoSugar
@@ -17,6 +21,8 @@ object ServiceTest {
       Future.value(new ConstantService[Int, Int](Future.value(2)))
 
     def close(deadline: Time): Future[Unit] = Future.Done
+
+    def status: Status = Status.Open
   }
 
   def await[A](fa: Future[A], timeout: Duration = 5.seconds): A =
@@ -110,6 +116,7 @@ class ServiceTest extends AnyFunSuite with MockitoSugar {
       def apply(conn: ClientConnection): Future[Service[Int, Int]] = Future.value(constSvc)
       def close(deadline: Time): Future[Unit] = Future.Done
       override def toString: String = "ServiceFactory"
+      def status: Status = constSvc.status
     }
     assert(svcFactoryWithToString.toString == "ServiceFactory")
   }
@@ -157,6 +164,7 @@ class ServiceTest extends AnyFunSuite with MockitoSugar {
     val factory = new ServiceFactory[String, String] {
       def apply(conn: ClientConnection) = Future.value(service)
       def close(deadline: Time) = Future.Done
+      def status: Status = service.status
     }
 
     verify(service, times(0)).close(any)

finagle-core/src/test/scala/com/twitter/finagle/client/DefaultPoolTest.scala

@@ -1,8 +1,14 @@
 package com.twitter.finagle.client
 
-import com.twitter.finagle.{Status, ClientConnection, Service, ServiceFactory}
+import com.twitter.finagle.Status
+import com.twitter.finagle.ClientConnection
+import com.twitter.finagle.Service
+import com.twitter.finagle.ServiceFactory
 import com.twitter.finagle.stats.InMemoryStatsReceiver
-import com.twitter.util.{Return, Await, Future, Time}
+import com.twitter.util.Return
+import com.twitter.util.Await
+import com.twitter.util.Future
+import com.twitter.util.Time
 import org.scalatest.funsuite.AnyFunSuite
 
 class DefaultPoolTest extends AnyFunSuite {
@@ -11,6 +17,8 @@ class DefaultPoolTest extends AnyFunSuite {
       Future.value(new MockService())
 
     override def close(deadline: Time): Future[Unit] = Future.Done
+
+    def status: Status = Status.Open
   }
 
   class MockService extends Service[Unit, Unit] {

finagle-core/src/test/scala/com/twitter/finagle/client/ExceptionRemoteInfoFactoryTest.scala

@@ -5,7 +5,9 @@ import com.twitter.finagle._
 import com.twitter.finagle.context.RemoteInfo
 import com.twitter.finagle.service.FailedService
 import com.twitter.finagle.tracing.Trace
-import com.twitter.util.{Await, Future, Time}
+import com.twitter.util.Await
+import com.twitter.util.Future
+import com.twitter.util.Time
 import java.net.InetSocketAddress
 import org.scalatestplus.mockito.MockitoSugar
 import org.scalatest.funsuite.AnyFunSuite
@@ -17,6 +19,7 @@ class ExceptionRemoteInfoFactoryTest extends AnyFunSuite with MockitoSugar {
     val failingFactory = new ServiceFactory[String, String] {
       def apply(conn: ClientConnection): Future[Nothing] = Future.exception(new HasRemoteInfo {})
       def close(deadline: Time): Future[Unit] = Future.Done
+      def status: Status = Status.Open
     }
 
     val downstreamAddr = new InetSocketAddress("1.2.3.4", 100)

finagle-core/src/test/scala/com/twitter/finagle/client/MethodBuilderTest.scala

@@ -611,6 +611,7 @@ class MethodBuilderTest
         Future.value(svc)
       def close(deadline: Time): Future[Unit] =
         svc.close(deadline)
+      def status: Status = Status.Open
     }
     val stk = Stack.leaf(Stack.Role("test"), svcFac)
     val stackClient = TestStackClient(stk, Stack.Params.empty)

finagle-core/src/test/scala/com/twitter/finagle/client/StackClientTest.scala

@@ -256,6 +256,7 @@ abstract class AbstractStackClientTest
         })
 
       def close(deadline: Time) = Future.Done
+      def status: Status = Status.Open
     }
 
     val stack = StackClient
@@ -300,6 +301,7 @@ abstract class AbstractStackClientTest
         })
 
       def close(deadline: Time) = Future.Done
+      def status: Status = Status.Open
     }
 
     val stack = StackClient
@@ -419,6 +421,7 @@ abstract class AbstractStackClientTest
         Failure.rejected("unable to establish session")
       )
       def close(deadline: Time) = Future.Done
+      def status: Status = Status.Open
     }
 
     intercept[Failure] { await(cl()) }
@@ -432,6 +435,7 @@ abstract class AbstractStackClientTest
         Failure("don't restart this!")
       )
       def close(deadline: Time) = Future.Done
+      def status: Status = Status.Open
     }
 
     intercept[Failure] { await(cl()) }
@@ -474,6 +478,7 @@ abstract class AbstractStackClientTest
 
       def apply(conn: ClientConnection): Future[Service[Unit, Unit]] = Future.value(service)
       def close(deadline: Time): Future[Unit] = Future.Done
+      def status: Status = service.status
     }
 
     val fac1 = new CountFactory
@@ -635,6 +640,7 @@ abstract class AbstractStackClientTest
               }
 
             def close(deadline: Time): Future[Unit] = Future.Done
+            def status: Status = Status.worst(endpoint1.status, endpoint2.status)
           }
         )
       )

finagle-core/src/test/scala/com/twitter/finagle/factory/ServiceFactoryCacheTest.scala

@@ -2,7 +2,11 @@ package com.twitter.finagle.factory
 
 import com.twitter.conversions.DurationOps._
 import com.twitter.finagle._
-import com.twitter.util.{Future, MockTimer, Time, Timer, Await}
+import com.twitter.util.Future
+import com.twitter.util.MockTimer
+import com.twitter.util.Time
+import com.twitter.util.Timer
+import com.twitter.util.Await
 import org.scalatestplus.mockito.MockitoSugar
 import org.scalatest.funsuite.AnyFunSuite
 
@@ -31,6 +35,8 @@ class ServiceFactoryCacheTest extends AnyFunSuite with MockitoSugar {
         factories -= i
         Future.Done
       }
+
+      def status: Status = Status.Open
     }
   }
 

finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/WeightedApertureTest.scala

@@ -64,6 +64,7 @@ abstract class BaseWeightedApertureTest(manageWeights: Boolean)
         })
 
       def close(when: Time): Future[Unit] = Future.Done
+      def status: Status = Status.Open
     }
   }
 

finagle-core/src/test/scala/com/twitter/finagle/naming/BindingFactoryTest.scala

@@ -62,6 +62,7 @@ case class Factory(i: Int) extends EndpointFactory[String, String] {
   override def close(deadline: Time): Future[Unit] = {
     Future.Done
   }
+  def status: Status = Status.Open
 }
 
 class BindingFactoryTest extends AnyFunSuite with MockitoSugar with BeforeAndAfter {
@@ -112,6 +113,7 @@ class BindingFactoryTest extends AnyFunSuite with MockitoSugar with BeforeAndAft
             tcOpt.foreach(_.advance(1234.microseconds))
             Future.value(Service.mk { _ => Future.value(bound.addr) })
           }
+          def status: Status = Status.Open
 
           def close(deadline: Time) = {
             closes += 1
@@ -237,6 +239,7 @@ class BindingFactoryTest extends AnyFunSuite with MockitoSugar with BeforeAndAft
             Future.exception(new NoBrokersAvailableException("/foo/bar"))
 
           def close(deadline: Time) = Future.Done
+          def status: Status = Status.Open
         }
       },
       Timer.Nil

finagle-core/src/test/scala/com/twitter/finagle/naming/NameTreeFactoryTest.scala

@@ -32,6 +32,8 @@ class NameTreeFactoryTest
           Future.value(null)
         }
         def close(deadline: Time) = Future.Done
+
+        def status: Status = Status.Open
       },
     Timer.Nil
   )
@@ -295,7 +297,7 @@ class NameTreeFactoryTest
           def apply(conn: ClientConnection): Future[Service[Unit, Unit]] =
             Future.value(Service.const(Future.Done))
           def close(deadline: Time) = Future.Done
-          override def status = Status.Open
+          def status = Status.Open
         },
       Timer.Nil
     )

finagle-core/src/test/scala/com/twitter/finagle/pool/BalancingPoolTest.scala

@@ -4,8 +4,12 @@ import com.twitter.conversions.DurationOps._
 import com.twitter.finagle._
 import com.twitter.finagle.client.Transporter
 import com.twitter.finagle.service.FailingFactory
-import com.twitter.util.{Await, Awaitable, Future, Time}
-import java.net.{InetAddress, InetSocketAddress}
+import com.twitter.util.Await
+import com.twitter.util.Awaitable
+import com.twitter.util.Future
+import com.twitter.util.Time
+import java.net.InetAddress
+import java.net.InetSocketAddress
 import org.scalatest.funsuite.AnyFunSuite
 
 object BalancingPoolTest {
@@ -39,6 +43,7 @@ object BalancingPoolTest {
               closes += 1
               Future.Done
             }
+            def status: Status = Status.Open
           }
         }
       }