status: avoid allocations when returning an OK status

[dzbarsky]

It's not expected for FromError to allocate when you have no error

[dfawley]

Would you mind also updating FromContextError as well? Thanks!

[dzbarsky]

@dfawley good point, looks like the implementations on nil Status do what we want. PTAL?

[dfawley]

Nice, thanks!

[dzbarsky]

https://travis-ci.org/grpc/grpc-go/jobs/563180848 seems like an infra flake? not sure how to tell travis to retry it

[dfawley]

https://travis-ci.org/grpc/grpc-go/jobs/563180848 seems like an infra flake? not sure how to tell travis to retry it

Yes, looks like it. I kicked travis.

[crosbymichael]

Isn't this a breaking change for the API? Clients that expect a non-nil OK status is currently getting nil now and can cause issues with server and clients on different versions

[dfawley]

Why is code broken whether the returned status is nil vs an allocated OK status? They should be functionally equivalent. Is your code explicitly checking for nil? If so, why?

[crosbymichael]

I'm just the messenger here :)

While I agree that nil and ok are equivalent in this context, this is still a breaking change. I'm guessing the nil check was for panics because it's not too common to call methods on a nil struct the way it's handled in go-grpc for the status type.

[dfawley]

I'm guessing the nil check was for panics because it's not too common to call methods on a nil struct the way it's handled in go-grpc for the status type.

Calls to methods on a nil *Status are entirely safe. I disagree this is an API change. Whether the return value is nil or not was never specified or guaranteed by the API, and all the same meaningful semantics of the API are preserved: a *Status is returned which represents the input error (Code() returns OK, etc).

Did the behavior change? Yes, but the same is true for bug fixes. Was this absolutely necessary? No, we could have done something similar differently, but I don't think it's reasonable or correct for applications to be checking for nil here, either (before or after this change!). Note that the previous implementation could literally never return nil, so such a check was dead code. I am sorry that the change broke someone, but I do believe it was a valid change.