Proposal: cleaning up exceptions

[mariusae]

The current exception hierarchy used by finagle has several problems:

• we must be careful to mark WriteException where applicable. Failing to do so leaves retry opportunities on the table;
• the provenance of exceptions is often confusing; SourcedExceptions are better, but again, this requires careful coordination;
• there are too many different types of exceptions, but they are rarely used to distinguish anything

And finally, a point of terminology: they aren't really exceptions — these aren't exceptional events; many operations fail regularly — they are failures.

I propose to change exceptions around a bit to:

1. Simplify the current exception hierarchy, reducing its cardinality;
2. Support orthogonal markers (is this exception retryable?);
3. Make it simple to understand where exceptions come from.

¶

We define a toplevel exception as a case class:

/**
 * Base exception for all Finagle-originated failures. These are
 * RuntimeExceptions, but with additional "sources" describing the
 * origins of the exception to aid in debugging.
 */
case class Failure(
  why: String,
  cause: Throwable = null,
  retryable: Boolean = false,
  interrupted: Boolean = false,
  sources: List[(String, Object)] = Nil,
  stacktrace: Array[StackTraceElement] = Failure.noStacktrace
) extends RuntimeException(why, cause) {
  def withSource(key: String, value: Object) = 
    copy(sources = (key, value) :: sources)

  def withStacktrace() =
    copy(stacktrace=Thread.currentThread.getStackTrace())

  def withRetryable(x: Boolean) = 
    if (x != retryable) copy(retryable=x) else this

  override def fillInStackTrace() = this
  def withStacktrace(): Failure =
    copy(stacktrace=Thread.currentThread().getStackTrace()))
}

It does not have any stack traces by default, and maintains a set of mutually exclusive labels (retryable, interrupted). Furthermore, it has a number of sources — key-value pairs. These are meant to reveal the provenance of the exception itself. A simple example is for the client to store the remote address of the host to which it was attempting to send a request ("remoteAddress", inetSocketAddress)

A number of convenience constructors and matchers are also provided, for example:

object Failure {
  object Retryable {
    def apply(why: String): Failure = 
      Failure(why, null, retryable = true)
    def apply(exc: Throwable): Failure =
      Failure(exc.getMessage, exc, retryable = true)

    def unapply(exc: Throwable): Option[Throwable] = exc match {
      case f@Failure(_, null, true, _, _, _) => Some(f)
      case Failure(_, cause, true, _, _, _) => Some(cause)
      case _ => None
    }
  }
}

So that exceptions can be constructed and taken apart easily:

val exc = Failure.Retryable("timed out in queue")
val shouldRetry = exc match {
  case Failure.Retryable(e) => true
  case => false
}

This has some neat properties:

1. Exceptions can be upgraded or downgraded. For example, it is safe to retry any ServiceFactory exception, so FactoryToService can simply rescue exceptions, returning exc.withRetryable(true).
2. We can insert filters where there is good provenance information. For example the host-specific stack can have a filter that adds a source indicating the SocketAddress of the host from which the failure originated. The same is true with client names: at the top of the Service stack, we can insert a filter sourcing the name of that client.

[tsuna]

I think this would be a nice improvement. Do you expect everything to be a Failure or will there be more case classes? I'm curious also as to whether it's better to put retryable and interrupted directly in Failure or to compose traits?

I like the sources, to essentially help tack on arbitrary annotations on exceptions/failures, but it also means that recipients of the exception will have to sort of inspect the sources for known keys they may care about, and because these are strings they may change and you wouldn't know, etc. Is there a way to retain this general purpose annotation mechanism yet provide a statically typed mechanism for important annotations that we expect to always be there (for instance the remote address of a socket from which an exception originated)?

[tsuna]

I think this would be a nice improvement. Do you expect everything to be a Failure or will there be more case classes? I'm curious also as to whether it's better to put retryable and interrupted directly in Failure or to compose traits?

I like the sources, to essentially help tack on arbitrary annotations on exceptions/failures, but it also means that recipients of the exception will have to sort of inspect the sources for known keys they may care about, and because these are strings they may change and you wouldn't know, etc. Is there a way to retain this general purpose annotation mechanism yet provide a statically typed mechanism for important annotations that we expect to always be there (for instance the remote address of a socket from which an exception originated)?

[bmatheny]

Like @tsuna I also think this is a nice improvement. My feedback would be to consider using a config class (like you do for builders and such) along with Failure. It seems like the list of options could grow beyond retryable and interrupted, and not having to change existing code to support that growth would be nice.

[bmatheny]

Like @tsuna I also think this is a nice improvement. My feedback would be to consider using a config class (like you do for builders and such) along with Failure. It seems like the list of options could grow beyond retryable and interrupted, and not having to change existing code to support that growth would be nice.

[mariusae]

@tsuna:

I like the sources, to essentially help tack on arbitrary annotations on exceptions/failures, but it also means that recipients of the exception will have to sort of inspect the sources for known keys they may care about, and because these are strings they may change and you wouldn't know, etc. Is there a way to retain this general purpose annotation mechanism yet provide a statically typed mechanism for important annotations that we expect to always be there (for instance the remote address of a socket from which an exception originated)?

I think that programmatic inspection will be rare; these will mostly be used for exception reporting and stats. In these cases, we can simply treat some names as special (remoteAddress, clientName) to recover useful provenance information.

[mariusae]

@tsuna:

I like the sources, to essentially help tack on arbitrary annotations on exceptions/failures, but it also means that recipients of the exception will have to sort of inspect the sources for known keys they may care about, and because these are strings they may change and you wouldn't know, etc. Is there a way to retain this general purpose annotation mechanism yet provide a statically typed mechanism for important annotations that we expect to always be there (for instance the remote address of a socket from which an exception originated)?

I think that programmatic inspection will be rare; these will mostly be used for exception reporting and stats. In these cases, we can simply treat some names as special (remoteAddress, clientName) to recover useful provenance information.

[mariusae]

@bmatheny:

Like @tsuna I also think this is a nice improvement. My feedback would be to consider using a config class (like you do for builders and such) along with Failure. It seems like the list of options could grow beyond retryable and interrupted, and not having to change existing code to support that growth would be nice.

Agreed: I may just make the case class constructor private, to enforce using the "builder style".

[mariusae]

@bmatheny:

Like @tsuna I also think this is a nice improvement. My feedback would be to consider using a config class (like you do for builders and such) along with Failure. It seems like the list of options could grow beyond retryable and interrupted, and not having to change existing code to support that growth would be nice.

Agreed: I may just make the case class constructor private, to enforce using the "builder style".

[mariusae]

@tsuna:

I think this would be a nice improvement. Do you expect everything to be a Failure or will there be more case classes? I'm curious also as to whether it's better to put retryable and interrupted directly in Failure or to compose traits?

The main reason for preferring a case class over mixins in this case is that it allows us to treat exceptions like a transformable value, without the need to resort to reflection or code generation tricks. This allows us to push sources and other annotations like "retryable" without altering the semantics of the exceptions:

service(req) rescue {
  case f: Failure =>
    Future.exception(f.withSource("mysource", blah))
}

[mariusae]

@tsuna:

I think this would be a nice improvement. Do you expect everything to be a Failure or will there be more case classes? I'm curious also as to whether it's better to put retryable and interrupted directly in Failure or to compose traits?

The main reason for preferring a case class over mixins in this case is that it allows us to treat exceptions like a transformable value, without the need to resort to reflection or code generation tricks. This allows us to push sources and other annotations like "retryable" without altering the semantics of the exceptions:

service(req) rescue {
  case f: Failure =>
    Future.exception(f.withSource("mysource", blah))
}

[mosesn]

This was implemented and rolled out a long time ago. We're migrating to Failure lazily. cc @roanta

[mosesn]

This was implemented and rolled out a long time ago. We're migrating to Failure lazily. cc @roanta