Support an optional ErrorCoder interface for Go servers

[cespare]

This is a proposal for an improvement to how Twirp handlers return errors. I originally brought this up a couple of years ago on the Twirp Slack in a discussion with @spenczar and @marwan-at-work.

To summarize, I propose that we add the following interface to the twirp package. A detailed proposal with background information follows.

// ErrorCoder may be implemented by third-party error types returned by
// Twirp handler functions to indicate a Twirp error code to report.
//
// If a non-nil error e returned by a handler is not an Error but is an
// ErrorCoder (or wraps one as indicated by errors.As), the server responds
// with an error using the code given by e.TwirpErrorCode and a message
// given by e.Error.
type ErrorCoder interface {
	TwirpErrorCode() ErrorCode
}

If this proposal is acceptable, I'm happy to send a PR.

---

Background

At my company we use Twirp extensively for making RPCs across systems. We sometimes need to convey particular error conditions (entity not found, say) and handle them differently on the client side. We can do this by sending a particular Twirp error code as the response.

The way this works today is that if a handler returns an error that implements twirp.Error, that Twirp error is used for the response. If the handler returns a non-nil error that does not implement twirp.Error, then it is considered a generic internal error and wrapped using twirp.InternalErrorWith.

This works well enough if the handler makes decisions about different error conditions directly. However, if the error is created a few functions down the call chain, it works less well:

• The function that generates the error might not be Twirp-specific, so it's not appropriate to return a twirp.Error, which is very much Twirp-specific. For instance, we have some servers that have shared internal logic which is used for generating web pages as well as Twirp responses and we don't want to generate Twirp errors in non-Twirp routes.
• The error generated down the call stack might be wrapped with other errors before the handler route gets it.

To work around this issue, a common pattern is for our servers to use a function to match errors to various internal error variables/types and return the appropriate Twirp error:

func toTwirpError(err error) error {
	switch {
	case errors.Is(err, errJobFinished):
		return twirp.NewError(twirp.FailedPrecondition, err.Error())
	case errors.Is(err, errNoLogDataAfterWait):
		return twirp.NewError(twirp.DeadlineExceeded, err.Error())
	case errors.As(err, new(jobNotFoundError)):
		return twirp.NotFoundError(err.Error())
	// ... several more cases ...
	default:
		return err
	}
}

All handlers have to remember to call toTwirpError any time they are returning the results of some function call that might return one of these internal errors. This is tedious and easy to mess up. It also happens to be fairly inefficient.

Proposal

We propose to add the ErrorCoder interface described above.

The idea is that the generated handler code will look something like this:

func writeError(ctx context.Context, resp http.ResponseWriter, err error, hooks *twirp.ServerHooks) {
	// If we already have a twirp.Error, use it.
	twerr, ok := err.(twirp.Error)
	if !ok {
		var ec twirp.ErrorCoder
		if errors.As(err, &ec) {
			// If we have a twirp.ErrorCoder, use the error code it
			// provides.
			twerr = twirp.NewError(ec.TwirpErrorCode(), err.Error())
		} else {
			// Otherwise, non-twirp errors are wrapped as Internal by default.
			twerr = twirp.InternalErrorWith(err)
		}
	}
	// ...
}

This allows us to create internal error types for each project which which are not Twirp-specific but can be turned into Twirp errors because they correspond to a Twirp error code. We can freely return those from any function whether or not it's being called as part of a Twirp route and even wrap them as they go up the call stack.

Backward compatibility

This is fully backward compatible and doesn't change the behavior of any existing code.

(Technically speaking, it could change the behavior of code which uses an error type which has a method TwirpErrorCode() twirp.ErrorCode, but that seems very unlikely.)

Alternatives

One slight variant on this interface which would be slightly more flexible but require a little more boilerplate to implement would be to have the optional method return a Twirp Error rather than just the ErrorCode:

type Errorer interface {
	TwirpError() Error
}

Naming

I think it's important that the method is called TwirpErrorCode, not ErrorCode, because the purpose of the interface is to be implemented by types outside of a Twirp context and so it's good for the method to make it clear that it is a Twirp-related adapter.

If the method is TwirpErrorCode, you could then argue that the interface should be TwirpErrorCoder, though it's a little redundant with the package name: twirp.TwirpErrorCoder. I don't have a strong feeling about this, though. (The interface itself should rarely be named in any code.)

[marwan-at-work]

My 2c so far:

I think we can accomplish the same thing by not adding new APIs and keeping the API surface small:

what if instead of adding a new interface, we just changed the generated code to go from:

twerr, ok := err.(twirp.Error)

To:

var twerr twirp.Error
ok := errors.As(err, &twerr)

This way, you don't need a new interface and your internal API can just implement twirp.Error, the same way it would have needed to implement twirp.ErrorCoder for the above proposal to work.

Also another alternative that doesn't require any changes in Twirp:

You can write a really small errors package that implements twirp.Error and supports wrapping. This way, you can pass "error codes" up the chain, wrap it with more errors, and still have the twirp-generated Go code be able to type assert the error code.

This is actually what I do for our internal work projects as well as my own side projects :)

[cespare]

@marwan-at-work you suggested that in our previous conversation, too, so I'll repeat some of the points I made there.

Unfortunately neither of these suggestions are satisfactory.

what if instead of adding a new interface, we just changed the generated code to go from: [...]

This way, you don't need a new interface and your internal API can just implement twirp.Error, the same way it would have needed to implement twirp.ErrorCoder for the above proposal to work.

For a start, "just" implementing twirp.Error is not a good idea. It is a large interface with 6 methods that we don't care about. The names of those methods (Msg, Code, etc) are too generic to be taken out of a Twirp context (for example, I might want my custom error to have a Code() int method to specify a particular HTTP status code). The Error string is supposed to return something about "twirp", which is not appropriate for a generic error type. That's what I meant in my proposal when I said the twirp.Error interface is Twirp-specific.

Furthermore, anyone implementing the twirp.Error interface needs to duplicate the twirp error metadata handling logic.

Therefore, like context.Context, twirp.Error is a type that is made to be used but not implemented by third-party packages. In our Slack conversation, @spenczar agreed with this characterization.

In contrast, my proposal is for an interface that exists only to be implemented by third-party packages, and that interface is very small (one method returning an error code). The name of the method includes "twirp" so this plays well with arbitrary error types that aren't Twirp-specific.

My solution is a bit like the json.Marshaler interface. The method name of Marshaler is MarshalJSON, not Marshal, so that my custom type can have MarshalJSON and MarshalBinary and whatever else. It's a minimal adapter to hook my type into the json package just like my ErrorCoder is a minimal adapter to hook my error type into the twirp package.

Finally, as a minor issue, your idea is less backward-compatible than my proposal. It's possible that people will be wrapping twirp errors already and the behavior of such handlers would change.

You can write a really small errors package that implements twirp.Error and supports wrapping.

I'm not interested in using a separate error-wrapping paradigm throughout the call stack of my programs just for Twirp. That is an invasive change. My proposal lets us write a single annotation (the method implementation) to indicate that an error corresponds to a particular Twirp error, and then not have to write any other code differently from our normal Go code.

[marioizquierdo]

Right. Sometimes it is useful to use a type that returns an Twirp error, instead of implementing the Twirp error interface.

I like the option of implementing TwirpError() twirp.Error for more flexibility. The extra boilerplate is not too much:

func (m *myStuff) TwirpError() twirp.Error {
    return twirp.NewError(m.code, m.err.Error())
}

[marioizquierdo]

Here's a PR adding the new interface: #320

Please take a look. If it looks good, I'll add tests and include it in the upcoming release v8.1.0 next week

[rhysh]

I think that the suggestion from @marwan-at-work to use errors.As needs more consideration. It looks to me like the code that service owners would write is very similar in either case, but that this proposal adds new Twirp-centric interfaces that we don't need.

I agree that the twirp.Error interface is bulky and not a good one to reimplement. It looks to me like errors.As allows use of that interface without reimplementing it.

type myError struct {
	base      error
	twirpCode string
	logKey    string
}

// As converts error representations in a general-purpose Go way.
//
// Proposed by @marwan-at-work, which I see as a good approach.
func (me *myError) As(dst interface{}) bool {
	switch dst := dst.(type) {
	case *twirp.Error:
		*dst = me.convertToTwirp()
		return true
	}
	return false
}

// TwirpError converts error representations in a Twirp-specific way.
//
// Presented in PR 320, which I don't see as the right approach.
func (me *myError) TwirpError() twirp.Error {
	return me.convertToTwirp()
}

// convertToTwirp does the necessary work of presenting an app-internal error in a
// format that Twirp can express on the wire.
func (me *myError) convertToTwirp() twirp.Error {
	var twerr twirp.Error
	switch {
	case errors.Is(me.base, errJobFinished):
		twerr = twirp.NewError(twirp.FailedPrecondition, me.base.Error())
	case errors.Is(me.base, errNoLogDataAfterWait):
		twerr = twirp.NewError(twirp.DeadlineExceeded, me.base.Error())
	case errors.As(me.base, new(jobNotFoundError)):
		twerr = twirp.NotFoundError(me.base.Error())
	// ... several more cases ...
	default:
		code := me.twirpCode
		if code == "" {
			code = twirp.Internal
		}
		twerr = twirp.NewError(code, me.base.Error())
	}

	twerr = twerr.WithMeta("log-key", me.logKey)
	// and whatever other Twirp-specific stuff you'd like

	return twerr
}

[cespare]

@rhysh this is a great point. I read @marwan-at-work's suggestion as being that myError should implement twirp.Error, which I objected to, but your idea to use the (unnamed) Aser interface avoids most of the issues.

I'd be happy enough with this solution. To be concrete, the idea is to simply change the generated code to use errors.As rather than a type assertion, correct?

One minor issue with this that I brought up in #300 (comment) is that this change is slightly less backward-compatible than adding a new interface. It's possible (perhaps unlikely) that people are wrapping twirp errors and don't want them to be unwrapped; the behavior would change for them. This isn't a problem for me; I just don't know how rigorous you folks want to be about preserving backward compatibility.

[marioizquierdo]

If a Twirp service calls another Twirp client, and the error handling (incorrectly) uses fmt.Errorf to wrap the client error, the service error will change if Twirp uses errors.As instead of direct casting. For example:

// Calling another Twirp service from a Twirp method handler
resp, clientErr := anotherService.MyMethod(ctx, req)
if clientErr != nil {
    // A non-twirp error that wraps a twirp error
    // becomes an internal error, but matching with errors.As would become the cilentError
    return nil, fmt.Errorf("otherService: %w", clientErr)
}

Folks should not be using fmt.Errorf like this, because the intentionality of a wrapped error is to keep the same type (see https://blog.golang.org/go1.13-errors#TOC_3.4.).

But the issue is that this type of code is not easy to identify. If Twirp were to be updated, there should be an easy way for services to safely update to the latest version. Maybe we can provide an error hook, or a flag that could be disabled. Let me try to do that in another PR.

Ultimately, Twirp should embrace good and simple Go code, and checking for the error type with errors.As is a better way to do it. But considering the tradeoffs for backwards compatibility, is not so clear what is the best way to move forward with it.

[marioizquierdo]

Here is the PR implementing errors.As: #323

[marioizquierdo]

Example of an error hook that could be used on a Twirp service before updating to use errors.As, to identify if there's a wrapped error that would return differently when updating to the new version of Twirp:

func CheckTwirpErrorCastBackwardsCompatibility(onDifferentError func(error, error)) *twirp.ServerHooks {
  return &twirp.ServerHooks{
    Error: func(ctx context.Context, twerr twirp.Error) context.Context {
      // Check if the error is an internal error with cause, which means that it was
      // likely returned as a non-twirp error by the service method handler.
      if twerr.Code() == twirp.Internal && twerr.Meta("cause") != "" {
        // Make sure it is a non-twirp error
        if err := errors.Unwrap(twerr); err != nil {
          if _, isTwerr := err.(twirp.Error); !isTwerr {
            // Check if the original non-twirp error was wrapping another twirp.Error inside
            var wrappedTwerr twirp.Error
            if errors.As(err, &wrappedTwerr) {
              // If this is the case, a Twirp service generated with the version with errors.As
              // would match and return the wrapped twirp error, while a service generated before
              // (using direct type cast) would return an internal error instead.
              // NOTE: if the service was explicitly using twirp.InternalErrorWith(err) then it is fine, it's a false positive.
              onDifferentError(twerr, wrappedTwerr)
            }
          }
        }
      }
      return ctx
    },
  }
}

It could cause false positives thought; maybe someone has explicitly wrapped an error that contains another twirp error inside. But if this hook doesn't catch any differences, then the service is safe; the update will not leak unintentionally wrapped twirp errors.

[cespare]

I prefer the errors.As version at this this point since it's a more minimal change which makes full use of the errors functionality and from my end it's equally convenient to the Errorer approach.

I don't have backward compatibility concerns for my own code, though, since I'm not nesting multiple twirp errors.

I'm not sure how likely this change would be to cause problems for people, but my guess is not that likely? The scenario would be:

• A twirp handler H is calling a separate twirp RPC
• H wraps the resulting error and returns that
• The callers of H depend on it returning an internal error for these cases rather than some other twirp error (or would do something different if they get a different error)

I could definitely believe that this would happen, but it doesn't seem like it would be all that common.

Instead of your error hook approach, how about adding a boolean flag to disable the error unwrapping? Though I guess I'm not exactly sure where that would go, except for inside ServerHooks, and it's not really a hook (since it's not a function).

@rhysh @marwan-at-work thoughts?

[marwan-at-work]

I agree that the errors.As solution is more elegant and I don't think the scenario above is quite likely. Twirp can mention this in the release notes in the off chance that someone is depending on a twirp error to always be internal. If Twirp wants to be super strict in behavior-related-compatibility, it can tag a new major version. I'm a fan of just mentioning this in the release notes as I agree that it's an uncommon scenario.

[marioizquierdo]

A serverOptions boolean flag is useful, but still not 100% safe. There's an exported method WriteError that could be used separately by middleware, that doesn't accept the same flag.

[marioizquierdo]

After thinking and discussing a little more, we decided to go with errors.As matching (PR #323)
This will be included in the upcoming v8.1 release 🎉