Skip to content

Commit

Permalink
finagle-core: Be Defensive when Converting Streams to Strings
Browse files Browse the repository at this point in the history
Problem

`RetryPolicy` converts a `Stream[Duration]` to a `String` in order
to have nice output for the representation of a `RetryPolicy`.
Unfortunately, we've run into instances where calling `toString` on a
`Stream` throws an `UnsupportedOperationException`. Given the stack
trace, this appears to be a Scala bug, and so we need to work around it.

Solution

Let's be defensive when calling `toString` on a `Stream[Duration]` inside of
`RetryPolicy`. By wrapping the call in a 'try/catch', we can return a
non-informative default value which only loses a little bit of
information.

JIRA Issues: CSL-10396

Differential Revision: https://phabricator.twitter.biz/D582199
  • Loading branch information
ryanoneill authored and jenkins committed Nov 20, 2020
1 parent c0f55b0 commit e7ec247
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 1 deletion.
7 changes: 7 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,13 @@ Runtime Behavior Changes
* finagle-core: Use Scala default implementation to calculate Hashcode and equals method for
ServiceFactoryProxy. ``PHAB_ID=D569045``

Bug Fixes
~~~~~~~~~

* finagle-core: Users should no longer see the problematic
`java.lang.UnsupportedOperationException: tail of empty stream` when a `c.t.f.s.RetryPolicy`
is converted to a String for showing. ``PHAB_ID=D582199``

20.10.0
-------

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import com.twitter.util.{Duration, Return, Throw, Try, TimeoutException => UtilT
import java.util.{concurrent => juc}
import java.{util => ju}
import scala.collection.JavaConverters._
import scala.util.control.NonFatal

/**
* A function defining retry behavior for a given value type `A`.
Expand Down Expand Up @@ -154,6 +155,21 @@ abstract class SimpleRetryPolicy[A](i: Int)

object RetryPolicy {

/**
* In theory, calling 'toString' on a `Stream` should always be safe.
* In practice, we've seen numerous occurrences of
* `java.lang.UnsupportedOperationException: tail of empty stream`
* when doing so. This method defensively handles `NonFatal` errors
* and returns `Stream(?)` as a result.
*/
private def streamToString[T](stream: Stream[T]): String = {
try {
stream.toString
} catch {
case NonFatal(_) => "Stream(?)"
}
}

// We provide a proxy around partial functions so we can give them a better .toString
private class NamedPf[A, B](name: String, f: PartialFunction[A, B])
extends PartialFunction[A, B] {
Expand Down Expand Up @@ -340,7 +356,7 @@ object RetryPolicy {
)(
shouldRetry: PartialFunction[A, Boolean]
): RetryPolicy[A] = {
RetryPolicy.named(s"backoff($backoffs)($shouldRetry)") { e =>
RetryPolicy.named(s"backoff(${RetryPolicy.streamToString(backoffs)})($shouldRetry)") { e =>
if (shouldRetry.applyOrElse(e, AlwaysFalse)) {
backoffs match {
case howlong #:: rest =>
Expand Down

0 comments on commit e7ec247

Please sign in to comment.