inject-utils: Move package private methods from PossiblyRetryable to …

…ExceptionUtils Problem The package private methods `PossiblyRetryable#isCancellation` and `PossibleRetryable#isNonRetryable` in inject-thrift are generally useful for processing interrupts on Futures. Making them publicly available allows users to not have to re-implement this logic (e.g., in the case they are attempting to mask cancellations on a Future). Solution Move the package private methods in `PossibleRetryable` in inject/inject-thrift to public methods in `ExceptionUtils` in inject/inject-utils. Result Less need for users to do this themselves (or hack around the visibility of these methods to call them). Differential Revision: https://phabricator.twitter.biz/D935287
twitter · Jul 16, 2022 · 3d35145 · 3d35145
1 parent fd39f10
commit 3d35145
Show file tree

Hide file tree

Showing 9 changed files with 105 additions and 62 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -7,6 +7,14 @@ Note that ``RB_ID=#`` and ``PHAB_ID=#`` correspond to associated message in comm
 Unreleased
 ----------
 
+Changed
+~~~~~~~
+
+* inject-utils|inject-thrift: Move package private methods `PossiblyRetryable#isCancellation` and
+  `PossibleRetryable#isNonRetryable` in inject-thrift to inject-utils `ExceptionUtils` as publicly
+  usable methods. These methods are generally useful when processing interrupts on Futures.
+  ``PHAB_ID=D935287``
+
 Runtime Behavior Changes
 ~~~~~~~~~~~~~~~~~~~~~~~~
 

diff --git a/build.sbt b/build.sbt
@@ -625,7 +625,7 @@ lazy val injectThrift = (project in file("inject/inject-thrift"))
     libraryDependencies ++= Seq(
       "org.apache.thrift" % "libthrift" % versions.libThrift exclude ("commons-logging", "commons-logging"),
       "com.twitter" %% "finagle-core" % versions.twLibVersion,
-      "com.twitter" %% "finagle-mux" % versions.twLibVersion,
+      "com.twitter" %% "finagle-mux" % versions.twLibVersion % Test,
       "com.twitter" %% "scrooge-core" % versions.twLibVersion,
       "com.twitter" %% "util-core" % versions.twLibVersion,
       "com.twitter" %% "util-mock" % versions.twLibVersion % Test,
@@ -669,6 +669,7 @@ lazy val injectUtils = (project in file("inject/inject-utils"))
     moduleName := "inject-utils",
     libraryDependencies ++= Seq(
       "com.twitter" %% "finagle-core" % versions.twLibVersion,
+      "com.twitter" %% "finagle-mux" % versions.twLibVersion,
       "com.twitter" %% "util-core" % versions.twLibVersion,
       "javax.xml.bind" % "jaxb-api" % versions.javaxBind,
       "org.slf4j" % "slf4j-simple" % versions.slf4j % "test-internal"

diff --git a/inject/inject-thrift/src/main/scala/BUILD b/inject/inject-thrift/src/main/scala/BUILD
@@ -10,7 +10,6 @@ scala_library(
     tags = ["bazel-compatible"],
     dependencies = [
         "finagle/finagle-core/src/main",
-        "finagle/finagle-mux/src/main/scala",
         "finatra/inject/inject-utils/src/main/scala",
         "scrooge/scrooge-core/src/main/scala",
         "util/util-core:scala",

diff --git a/inject/inject-thrift/src/main/scala/com/twitter/inject/exceptions/PossiblyRetryable.scala b/inject/inject-thrift/src/main/scala/com/twitter/inject/exceptions/PossiblyRetryable.scala
@@ -1,15 +1,12 @@
 package com.twitter.inject.exceptions
 
-import com.twitter.finagle.mux.ClientDiscardedRequestException
-import com.twitter.finagle.{
-  CancelledConnectionException,
-  CancelledRequestException,
-  Failure,
-  FailureFlags,
-  service => ctfs
-}
-import com.twitter.finagle.service.{ReqRep, ResponseClass}
-import com.twitter.util.{Return, Throw, Try}
+import com.twitter.finagle.{service => ctfs}
+import com.twitter.finagle.service.ReqRep
+import com.twitter.finagle.service.ResponseClass
+import com.twitter.inject.utils.ExceptionUtils
+import com.twitter.util.Return
+import com.twitter.util.Throw
+import com.twitter.util.Try
 import scala.util.control.NonFatal
 
 /**
@@ -65,24 +62,6 @@ object PossiblyRetryable {
    * @return true if the [[Throwable]] represents an Exception or Failure which is possibly
    *         retryable, false otherwise.
    */
-  def possiblyRetryable(t: Throwable): Boolean = {
-    !isCancellation(t) && !isNonRetryable(t) && NonFatal(t)
-  }
-
-  private[inject] def isCancellation(t: Throwable): Boolean = t match {
-    case _: CancelledRequestException => true
-    case _: CancelledConnectionException => true
-    case _: ClientDiscardedRequestException => true
-    case f: FailureFlags[_] if f.isFlagged(FailureFlags.Ignorable) => true
-    case f: FailureFlags[_] if f.isFlagged(FailureFlags.Interrupted) => true
-    case f: Failure if f.cause.isDefined => isCancellation(f.cause.get)
-    case _ => false
-  }
-
-  private[inject] def isNonRetryable(t: Throwable): Boolean = t match {
-    case _: NonRetryableException => true
-    case f: FailureFlags[_] if f.isFlagged(FailureFlags.Ignorable) => true
-    case f: Failure if f.cause.isDefined => isNonRetryable(f.cause.get)
-    case _ => false
-  }
+  def possiblyRetryable(t: Throwable): Boolean =
+    !ExceptionUtils.isCancellation(t) && !ExceptionUtils.isNonRetryable(t) && NonFatal(t)
 }
diff --git a/...ect-thrift/src/test/scala/com/twitter/inject/tests/exceptions/PossiblyRetryableTest.scala b/...ect-thrift/src/test/scala/com/twitter/inject/tests/exceptions/PossiblyRetryableTest.scala
@@ -1,16 +1,11 @@
 package com.twitter.inject.tests.exceptions
 
-import com.twitter.finagle.mux.ClientDiscardedRequestException
-import com.twitter.finagle.{
-  CancelledConnectionException,
-  CancelledRequestException,
-  Failure,
-  FailureFlags
-}
+import com.twitter.finagle.CancelledRequestException
 import com.twitter.inject.Test
 import com.twitter.inject.exceptions.PossiblyRetryable
 import com.twitter.scrooge.ThriftException
-import com.twitter.util.{Return, Throw}
+import com.twitter.util.Return
+import com.twitter.util.Throw
 
 class PossiblyRetryableTest extends Test {
 
@@ -19,18 +14,6 @@ class PossiblyRetryableTest extends Test {
       extends ThriftException
       with com.twitter.inject.exceptions.NonRetryableException
 
-  test("test isCancellation") {
-    assertIsCancellation(new CancelledRequestException)
-    assertIsCancellation(new CancelledConnectionException(new Exception("cause")))
-    assertIsCancellation(new ClientDiscardedRequestException("cause"))
-    assertIsCancellation(Failure("int", FailureFlags.Interrupted))
-    assertIsCancellation(Failure.rejected("", new CancelledRequestException))
-  }
-
-  test("test isNonRetryable") {
-    assertIsNonRetryable(Failure("int", FailureFlags.Ignorable))
-  }
-
   test("test apply") {
     PossiblyRetryable(PossiblyRetryableException) should be(true)
     PossiblyRetryable(NonRetryableException) should be(false)
@@ -71,12 +54,4 @@ class PossiblyRetryableTest extends Test {
       true
     )
   }
-
-  private def assertIsCancellation(e: Throwable): Unit = {
-    PossiblyRetryable.isCancellation(e) should be(true)
-  }
-
-  private def assertIsNonRetryable(e: Throwable): Unit = {
-    PossiblyRetryable.isNonRetryable(e) should be(true)
-  }
 }
diff --git a/inject/inject-utils/src/main/scala/BUILD b/inject/inject-utils/src/main/scala/BUILD
@@ -17,6 +17,7 @@ scala_library(
         "3rdparty/jvm/joda-time",
         "3rdparty/jvm/org/slf4j:slf4j-api",
         "finagle/finagle-core/src/main",
+        "finagle/finagle-mux/src/main/scala",
         "finatra/inject/inject-core/src/main/scala/com/twitter/inject",
         "util/util-core:scala",
         "util/util-slf4j-api/src/main/scala/com/twitter/util/logging",

diff --git a/inject/inject-utils/src/main/scala/com/twitter/inject/utils/ExceptionUtils.scala b/inject/inject-utils/src/main/scala/com/twitter/inject/utils/ExceptionUtils.scala
@@ -1,8 +1,18 @@
 package com.twitter.inject.utils
 
-import com.twitter.finagle.{FailedFastException, SourcedException, TimeoutException}
+import com.twitter.finagle.CancelledConnectionException
+import com.twitter.finagle.CancelledRequestException
+import com.twitter.finagle.Failure
+import com.twitter.finagle.FailureFlags
+import com.twitter.finagle.FailedFastException
+import com.twitter.finagle.SourcedException
+import com.twitter.finagle.TimeoutException
+import com.twitter.finagle.mux.ClientDiscardedRequestException
+import com.twitter.inject.exceptions.NonRetryableException
 import com.twitter.util.Throwables._
-import com.twitter.util.{Throw, Try}
+import com.twitter.util.Throw
+import com.twitter.util.Try
+import scala.annotation.tailrec
 
 object ExceptionUtils {
 
@@ -42,4 +52,41 @@ object ExceptionUtils {
     case Throw(e) => toExceptionDetails(e) + " " + toExceptionMessage(e)
     case _ => ""
   }
+
+  /**
+   * Determines if the given [[Throwable]] represents a "Cancellation". "Cancellations" are not
+   * typically meant to be retried as they represent an interrupt that signals the caller is
+   * no longer listening for the response to the work being done. This method can be useful in
+   * cases where users wish to "mask" cancellations to asynchronous work because they desire the
+   * work to finish regardless of whether the caller is still listening. e.g., work which is
+   * side-effecting and should not be interrupted once started.
+   * @param t the [[Throwable]] to inspect to determine if it represents a "cancellation".
+   * @return `true` if the given [[Throwable]] represents a "cancellation", `false` otherwise.
+   * @see [[com.twitter.util.Future.mask]]
+   * @see [[https://twitter.github.io/finagle/guide/FAQ.html?highlight=cancelled#what-are-cancelledrequestexception-cancelledconnectionexception-and-clientdiscardedrequestexception]]
+   */
+  @tailrec
+  def isCancellation(t: Throwable): Boolean = t match {
+    case _: CancelledRequestException => true
+    case _: CancelledConnectionException => true
+    case _: ClientDiscardedRequestException => true
+    case f: FailureFlags[_] if f.isFlagged(FailureFlags.Ignorable) => true
+    case f: FailureFlags[_] if f.isFlagged(FailureFlags.Interrupted) => true
+    case f: Failure if f.cause.isDefined => isCancellation(f.cause.get)
+    case _ => false
+  }
+
+  /**
+   * Determines if the given [[Throwable]] represents an exception which should be considered as
+   * signaling that the work which caused the exception should not be retried.
+   * @param t the [[Throwable]] to inspect to determine if it represents a signal to not retry
+   * @return `true` if the given [[Throwable]] represents an non-retryable exception, `false` otherwise.
+   */
+  @tailrec
+  def isNonRetryable(t: Throwable): Boolean = t match {
+    case _: NonRetryableException => true
+    case f: FailureFlags[_] if f.isFlagged(FailureFlags.Ignorable) => true
+    case f: Failure if f.cause.isDefined => isNonRetryable(f.cause.get)
+    case _ => false
+  }
 }
diff --git a/inject/inject-utils/src/test/scala/BUILD b/inject/inject-utils/src/test/scala/BUILD
@@ -12,6 +12,7 @@ junit_tests(
             scope = "runtime",
         ),
         "finagle/finagle-core/src/main",
+        "finagle/finagle-mux/src/main/scala",
         "finatra/inject/inject-core/src/main/scala/com/twitter/inject",
         "finatra/inject/inject-core/src/test/scala/com/twitter/inject",
         "finatra/inject/inject-utils/src/main/scala",

diff --git a/.../inject-utils/src/test/scala/com/twitter/inject/tests/exceptions/ExceptionUtilsTest.scala b/.../inject-utils/src/test/scala/com/twitter/inject/tests/exceptions/ExceptionUtilsTest.scala
@@ -0,0 +1,32 @@
+package com.twitter.inject.tests.exceptions
+
+import com.twitter.finagle.CancelledConnectionException
+import com.twitter.finagle.CancelledRequestException
+import com.twitter.finagle.Failure
+import com.twitter.finagle.FailureFlags
+import com.twitter.finagle.mux.ClientDiscardedRequestException
+import com.twitter.inject.Test
+import com.twitter.inject.utils.ExceptionUtils
+
+class ExceptionUtilsTest extends Test {
+
+  test("test isCancellation") {
+    assertIsCancellation(new CancelledRequestException)
+    assertIsCancellation(new CancelledConnectionException(new Exception("cause")))
+    assertIsCancellation(new ClientDiscardedRequestException("cause"))
+    assertIsCancellation(Failure("int", FailureFlags.Interrupted))
+    assertIsCancellation(Failure.rejected("", new CancelledRequestException))
+  }
+
+  test("test isNonRetryable") {
+    assertIsNonRetryable(Failure("int", FailureFlags.Ignorable))
+  }
+
+  private def assertIsCancellation(e: Throwable): Unit = {
+    ExceptionUtils.isCancellation(e) should be(true)
+  }
+
+  private def assertIsNonRetryable(e: Throwable): Unit = {
+    ExceptionUtils.isNonRetryable(e) should be(true)
+  }
+}