bug: Prevent nil-pointer when using custom errors from sub function

[elgohr]

Fixing the following scenario: When returning nil as *apiresponses.FailureResponse from a sub function, the broker can't handle it and produces a nil pointer error.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/185322738

The labels on this github issue will be updated when the story is started.

[blgm]

This fixes a curious behaviour that I can't remember coming across, although it is mentioned in the Go FAQ.

I have a slight hesitation in adding reflect.ValueOf(err).IsNil() to the error checking process. This is because it:

• adds complexity
• reflect is slow (though given HTTP is being used, it may not make a practical difference)
• IsNil() can panic in some situations, so we might be fixing one problem only to introduce another
• it's not idiomatic Go. For example, I can't see examples of this in the Go standard library.

According to the Go FAQ:

It's a good idea for functions that return errors always to use the error type in their signature (as we did above) rather than a concrete type such as *MyError, to help guarantee the error is created correctly. As an example, os.Open returns an error even though, if not nil, it's always of concrete type *os.PathError.

I'm in two minds about this. On one hand, I like the idea of brokerapi being robust and handling situations like this.

On the other hand, using reflect in error checks is not idiomatic Go. For example, filepath.WalkDir() in the standard library does just does an err == nil check, and would presumably be susceptible to the same issue. So it could be argued that this is a very contrived situation and the real error is that subFunctionWithCustomError() should return error and not a concrete type.

These are just my personal thoughts, and I could be persuaded in either direction. I'd be interested to hear what other folks think, and whether there are good examples or precedents to follow.

[elgohr]

I would be more comfortable with something slow (when using custom errors) than crashing.
Whether this is idiomatic go or not had been discussed a lot (https://groups.google.com/g/golang-nuts/c/wnH302gBa4I).
To my point of view form follows function.

[blgm]

I was reflecting on this, and part of the challenge for me is that if we replace:

if err != nil {

with

err != nil && !reflect.ValueOf(err).IsNil() {

in one place, then potentially this change should be made in all Go code. And even the basic tutorials should be updated. I see that there's a debate about whether this is a design bug in Go, and I don't want to get into that debate because Go is not about to change. I understand how the change fixes the bug that you observed, but I hope you can understand that I'm hesitant about something like this as it would look unusual to a seasoned Go developer.

So I tried to think of alternatives that don't have such broad ecosystem implications. Clearly there was a problem that led to this PR being created, and it would be good to make that problem better.

My guess is that this originally came from a function that looked a bit like this:

func(_ context.Context, _ string, _ domain.ProvisionDetails, _ bool) (domain.ProvisionedServiceSpec, error) {
  var err *apiresponses.FailureResponse
  // some stuff that assigns to err if there was an error
  return domain.ProvisionedServiceSpec{}, err
}

This function always causes a panic (unless err is assigned).

I think there are two problems here:

1. A nil apiresponses.FailureResponse panics when any method is called
2. While writing var err *apiresponses.FailureResponse is completely understandable, it should actually be written var err error, and the panic would not occur. This is counterintuitive, but it's called out as a gotcha in the Go FAQ and various blogs about Go. Ideally a linter (like go vet) could identify issues like this and highlight them in the IDE, but I haven't seen such a linter yet.

I've put together a draft PR that addresses (1), so that if a nil apierrors.FailureResponse() is created, then it will not panic when methods on it are called. In the situation above, it would result in a Provision operation always failing with an "uninitialized FailureResponse" error, which would hopefully lead someone to revise their code.

What do you think? I realise it's not exactly what you wanted, but hopefully it's a step in the right direction.

[blgm]

Also I think another bug is this function definition in brokerapi:

func NewFailureResponse(err error, statusCode int, loggerAction string) *FailureResponse

It should be:

func NewFailureResponse(err error, statusCode int, loggerAction string) error

And then it would discourage folks from having *apiresponses.FailureRespose pointers in the first place. This one is harder to fix and would likely need to wait until v11.

[elgohr]

I guess the change in NewFailureResponse should already help a lot. Regarding the nil-check within the error I would prefer a static error that is well documented.

[blgm]

@elgohr

Regarding the nil-check within the error I would prefer a static error that is well documented.

Could you expand on that a bit. I want to be sure that I've understood you correctly.

[blgm]

My current plan is to resolve this with PR #257

I speculated that maybe this problem first occurred for you when writing some code like:

func(_ context.Context, _ string, _ domain.ProvisionDetails, _ bool) (domain.ProvisionedServiceSpec, error) {
  var err *apiresponses.FailureResponse
  if someOperationFailed() {
		err = apiresponses.NewFailureResponse(errors.New("something bad has happened with the universe"), http.StatusInternalServerError, "log-key")
  }
  return domain.ProvisionedServiceSpec{}, err
}

The PR changes the return type of NewFailureResponse() to error so the code above would no longer compile, and you'd most likely write var err error instead of having any *apiresponses.FailureResponse types in your code. It has the advantages of:

• making NewFailureResponse() be in line with what the Go FAQ recommends for error return types
• it means that traditional if err != nil checks will work as expected without needing reflection

Because it can break compilation in some circumstances, I'm going to get some second opinions on this as the aim is to fix problems without creating new problems.