encoding.GetCodec(proto.Name) broke in 1.66.0

[ash2k]

What version of gRPC are you using?

1.66.0

What version of Go are you using (go version)?

1.23.0

What operating system (Linux, Windows, …) and version?

macOS

What did you do?

import (
  "google.golang.org/grpc/encoding/proto"
  "google.golang.org/grpc/encoding"
)

var protoCodec = encoding.GetCodec(proto.Name)

What did you expect to see?

Valid proto codec in the variable.

What did you see instead?

Updated from 1.65.0 to 1.66.0 and tests panicked.

The variable is nil.

This is obviously a breaking change that will affect people. I don't mind, I'm going to change my code to use v2 codecs, so it's more of a FYI. Really looking forward to the performance improvements of new codecs.

[arjan-bal]

We tried our best to ensure backward compatibility while making this change. We needed the v2 proto codec to be used by default. Initially we tried to have both v1 and v2 proto codecs exist, while giving priority to the v2 codec with the same name (if it existed). This broke some users who were replacing the v1 proto codec since that wasn't getting used after replacement. So #7557 was made to have a common encoder registry.

For this particular case, I guess the fix would be to return a wrapper for the v2 proto codec that adapts it to the v1 codec interface. Assigning to @dfawley for commenting on the feasibility of this.

Having said that, since the encoding package is experimental, we are leaning towards not fixing this due to the narrow impact.

[ash2k]

I think the root of the issue is the unfortunate choice of the "registry" pattern. As I said in a comment here, would be much better to not have this global mutable state. If it was the case, this bug wouldn't exist. Anyone, who needed a custom codec, would instantiate it (via a constructor function or type literal) and use as required. No registry == not possible to rely on it, not possible to break those who rely on it. Today I have to use the registry to get an instance of the codec by name, no other way to do it.

Since this package is experimental, maybe the codec v2 is our chance to not reintroduce the "registry" pattern again? Leave codec v1 as it is, and use per-server codecs, defaulting to the v2 proto codec.

There is ForceServerCodec() today. A difference in how it works vs the registry is that registry allows to have multiple codecs vs ForceServerCodec() makes the server only support this single codec. Not a problem for what I do, but it's less flexible.

I don't insist on this, just trying to understand why the existing pattern is used/in place and offer a different approach. I may be overlooking some use case.

One use case is, perhaps, allowing an import of a package to register a codec (from init()). However, in my opinion, allowing a third party package to influence how a program works just by importing the package, is not a great idea. In my experience this often leads to unexpected problems.

[dfawley]

The registry is pretty integral to the way the codecs work. They are registered and then accessed on the receiving path. Users are not meant to access the registry (it's meant for gRPC to consume), so probably the biggest mistake was not putting the GetCodec function in an internal package.

[ash2k]

Since you just broke all users of that function anyway, maybe it's a chance to move it to internal then to let everyone know it's not for public consumption?

[dfawley]

We would need to mark it as deprecated first. Note that we only broke users that were calling it with "proto".

[redloaf]

Please consider reopening this and addressing the breakage. The users who were calling it with proto seem to be numerous and high-profile: kubernetes, loki, docker, pulumi, kaniko, kops, k6, podman, nginx.

I agree these projects probably shouldn't be relying on an experimental API, but as noted, there is no other way to access this codec. Would you please either make this change backward-compatible or provide a more stable, supported means of obtaining the codecs that 700+ projects are currently obtaining from this registry via an experimental API?

[redloaf]

I see now that much of that usage is vendored and is grpc calling itself, so the number is actually much smaller. It's still ~50 projects, and pulumi is still in the list, though the others I mentioned are not. Here's a corrected search. Even then, some of those appear to be calls from forks/copies/GOPATH versions where the library is calling itself.

I still think it's worthwhile to reconsider this.