Using InternalMetadata class in Armeria

[anuraaga]

Hi all.

As some of you may have seen, Armeria team mantains a Java implementation of gRPC that runs on top of the Armeria server framework, a generic HTTP/1+2 server framework built on Netty (made by our good friend @trustin himself :) )

We interface with protoc stubs by creating Call objects and starting them - after started, a Call goes through io.grpc business logic as normal in most cases with surprisingly few gotchas. The Call interface is a quite nice plugin point.

https://github.com/line/armeria/blob/master/grpc/src/main/java/com/linecorp/armeria/server/grpc/ArmeriaServerCall.java

We have one major caveat though, no support for the Metadata class

https://github.com/line/armeria/blob/master/grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcService.java#L77

While Armeria has its own methods for adding response trailers, we're finding more and more users would like to be able to use it so their existing gRPC business logic can run as-is on Armeria, and if possible I'd like to achieve that. It means I need to use InternalMetadata to be able to create and serialize Metadata.

Is it kosher for Armeria to use this class? I understand it is for "specifically supported transport packages" - if Armeria could be considered a specifically supported transport package, that would be great! But if it's not possible to have official support like that, if it's "at your own risk but should be ok" that'd be fine too. For context, the first version of our implementation used a lot of io.grpc.internal classes to try to minimize code duplication - that was naturally a nightmare to maintain and I'm hoping not to repeat my mistake ;)

Also, as an alternative or in addition, any thoughts on making Metadata an interface and letting implementations control the serialization themselves? If that happened, we wouldn't need to use InternalMetadata either.

Thanks!

References

• One of several slack threads
• armeria-grpc does not support gRPC Metadata line/armeria#1788
• Marshal headers <-> gRPC Metadata. line/armeria#1790

[dapengzhang0]

Is that all you need is public static byte[][] InternalMetadata.serialize(Metadata md)? Seems all other internal APIs in InternalMetadata can be implemented using public APIs following the spec.

byte[][][ serialize(Metadata md) can also be implemented using public APIs keys() and getAll() although that would be not efficient and would not preserve the original order.

[carl-mastrangelo]

@ejona86 can correct me if I am wrong, but I think it would be okay if you pin to a specific version of gRPC. io.grpc.Internal* and io.grpc.internal.* are not subject to the same API compatibility rules as the rest of gRPC, so you would be signing up to fix this as we break things. That said, the InternalMetadata API hasn't changed in a while.

[ejona86]

Seems just newMetadata() and serialize() are needed. Armeria is definitely the type of project that needs to use those sorts of APIs.

So there's two reasons they are internal: byte[] mutability and API fitness. byte[] mutability is just because we treat keys/values as immutable arrays. We can open up the API while still saying, "you had better use this correctly." API fitness is just that the API is barely good enough for our own internal use; we'd be fine iterating on it and improving things. So if Armeria has suggestions, I'd be interested.

I guess this sort of means we'll have Internal (our use only) and Stable-but-not-for-normal-use APIs. I do think these should be able to move out of Internal, once we know how they should look.

[ejona86]

It is possible to avoid these two APIs, but serialize() in particular would be slow to avoid. newMetadata() can be faked with some "trash" keys, at the cost of some verification for ASCII values. We do need an API that makes iterating over all keys cheaper than the n^2 cost when using keys().

[anuraaga]

Thanks for this discussion. Indeed, newMetadata and serialize are the two important methods (I used the base64 encoder in the linked PR but it's very easy to remove that).

For reference, Armeria uses the unsafe word to indicate APIs that are advanced and dangerous / hard to use but still supported. I borrowed the practice from protobuf. It might make sense here if going with exposing Stable-but-not-for-normal-use APIs. Aside from Metadata, I'm sure some grpc-java users would appreciate an unsafe way to access aliased bytestrings without copies.

I did come to the same conclusion that it is possible to use the public APIs by converting all header names into Key but it's very obtuse, and probably very slow even if escape analysis kicks in. So hoping to avoid that :)

As far as API suggestions, some that come to mind are

• I was surprised that Metadata didn't handle making values ASCII and the transport layer does it (notably base encoding binary values). Maybe it's because while the gRPC wire format defines Custom-Metadata to only have ASCII values, the class Metadata maybe doesn't have that restriction. But I was sort of expecting it to.

• It was unclear whether newMetadata should be passed all headers or only ones that don't start with : or grpc-. Based on the spec I would expect the latter but couldn't find that logic in the grpc-java transports. In a fit API it would be nice if that's clearer.

• Having an API that returns an iterator of both raw header names and values would solve a lot. Armeria would use it when serializing but I think users could also find it useful. Is there a way for them right now to log Metadata in a generic way without keys (trash keys work but are quite hard to reason about)?

• Could investigate the real value of using byte[][] instead of String[]. APIs like an iterator become much more practical if doing so. I'd expect it to have similar performance on Java 11 but it's true that Java 8 might suffer.

• The above points become moot since Armeria would have to implement all Metadata logic, but I think it could make a lot of sense if Metadata became an interface with the public APIs and the implementation class stayed internal. Then transports could implement it as they see fit (e.g., it might make sense to back Metadata with Netty's Http2Headers itself)

Hope this info helps. Let me know if I can help with anything else. If we decide on a direction I'd be happy to send a PR for it too.

[ejona86]

Unsafe is probably the appropriate word to use.

I was surprised that Metadata didn't handle making values ASCII and the transport layer does it (notably base encoding binary values)

This is because base64-encoding is transport-specific. C has an HTTP/2 extension that allows binary headers and if there were any high-performance RDMA-based transport or such wouldn't want to use base64 either. Basically, base64 is dependent on the wire protocol and thus in the transport.

Maybe it's because while the gRPC wire format defines Custom-Metadata to only have ASCII values

ASCII keys must have ascii values, and we do verify that within Metadata. But binary keys are arbitrary binary.

It was unclear whether newMetadata should be passed all headers or only ones that don't start with : or grpc-. Based on the spec I would expect the latter but couldn't find that logic in the grpc-java transports.

It should provide the "custom metadata" to Metadata. I don't think the behavior is clearly defined anywhere, but the "best" behavior is to remove :, and all hop-by-hop headers from HTTP (which includes things listed in the Connection header). Even things like content-type would be fair game. grpc- is a bit complicated right now; there's things that require it, so you can leave them in.

Could investigate the real value of using byte[][] instead of String[].

We totally can't use a string for the values. Binary values should be binary. But also Metadata internally keeps everything in a byte[][], so byte[] is cheap. There's some possibility for changes here, but it's all byte-based since that was what was best for Netty.

I'd expect it to have similar performance on Java 11 but it's true that Java 8 might suffer.

Yeah... the only thing using Java 11 that I touch is Bazel. So Java 8 is still the primary performance target.

I think it could make a lot of sense if Metadata became an interface

That not really on the table as it would break basically all the APIs. "Users" create Metadata directly via new Metadata() and similar when sending. Yes, you could maybe implement your own for receiving. Instead of going full interface I think we'd have to let you specify a "storage" for Metadata.

[ejona86]

"hop-by-hop" I mean things described at https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.5.1 . Yes, I know that's not the most recent RFC, but I could find it quickly.

[anuraaga]

This is because base64-encoding is transport-specific. C has an HTTP/2 extension that allows binary headers and if there were any high-performance RDMA-based transport or such wouldn't want to use base64 either. Basically, base64 is dependent on the wire protocol and thus in the transport.

Could you explain this a bit more? You're saying some transports transfer binary without base64 encoding? This makes sense to me since HTTP/2 does support binary natively, but when I look at the gRPC spec it says Binary-Header → {Header-Name "-bin" } {base64 encoded value} so I was expecting that base64 must always used.

Don't mind implementing the base64 in the transport, just want to make sure I'm understanding the spec correctly.

That not really on the table as it would break basically all the APIs. "Users" create Metadata directly via new Metadata()

Ah forgot about this, yeah it's definitely not practical.

So I guess to move forward, I'd say that the current APIs that take byte[][] and return byte[][] are find for Armeria's use case. They do make sense since Metadata is just a lot of binary data - and to reduce copying if these were exposed in an "unsafe" way that documented the caveats that the arrays must not be modified, then we'd be fine.

[ejona86]

but when I look at the gRPC spec it says Binary-Header → {Header-Name "-bin" } {base64 encoded value} so I was expecting that base64 must always used.

So in normal HTTP/2, yes, that is how it must be done. There is an HTTP/2 extension that that C is using. Basically, you can negotiate support for "true binary" headers and then if the header value starts with \0 then everything after that first byte is "true binary". Semantically it is equivalent to a base64-encoded value, so if a proxy needed to propagate it to a peer that didn't support the extension the proxy could base64 the value at that point. So it is really a separate discussion and shouldn't cause you concern.

So I guess to move forward, I'd say that the current APIs that take byte[][] and return byte[][] are find for Armeria's use case.

@carl-mastrangelo, do you agree that we should probably expose roughly the same API we have today? I think the only change would be to expose a factory method instead of new Metadata(byte[][]) constructor.

[carl-mastrangelo]

Sounds fine to me, with caveats:

• We probably need to make defensive copy of the args for outside users. Even if Armeria plays by the rules, it's too easy for other people to not. I think this issue is about API compatibility, so safety comes first.

• The names of the methods should probably indicate that it's meant for transports, not so much end users. I kinda like how InternalMetadata prevents normal Metadata users from being able to tab-complete the method names. Maybe we could make a TransportMetadata class, which is experimental?

[ejona86]

We probably need to make defensive copy of the args for outside users.

Sounds good. And that will mirror serialize().

The names of the methods should probably indicate that it's meant for transports, not so much end users.

I thought we'd put "unsafe" in the names, and then documentation describe it is really only usable by transports. I don't think I want to start creating many Transport* classes in the stable API; I'd rather it not show up that high-level in the javadoc. Unless it was a single UnsafeMethods/TransportMethods class which exposed many such methods.

Were you thinking using a separate class to hide it from the majority of Bazel users?