Add a gRPC Web to HTTP/2 codec #1047
Conversation
| ) -> GRPCWebToHTTP2ServerCodec.StateMachine.Action { | ||
| switch self { | ||
| case .idle: | ||
| var headers = HPACKHeaders(httpHeaders: head.headers, normalizeHTTPHeaders: true) |
There was a problem hiding this comment.
Should we reserve capacity here?
| case .idle: | ||
| var headers = HPACKHeaders(httpHeaders: head.headers, normalizeHTTPHeaders: true) | ||
| headers.add(name: ":path", value: head.uri) | ||
| headers.add(name: ":method", value: head.method.rawValue) |
There was a problem hiding this comment.
These pseudo-headers are surely not going to be in the right place in the block, right? And I think we're missing two: :authority and :scheme are both expected as well.
There was a problem hiding this comment.
I didn't think the order mattered given these will only be read by the HTTP/2 to gRPC codec? Easy enough to change though.
I wasn't sure about including :authority and :scheme; they're also not required but I suppose they might be expected by a user in e.g. an interceptor. WDYT?
There was a problem hiding this comment.
I'm inclined to suggest that we should do the right thing here. It reduces the risk that we might accidentally introduce a component that doesn't expect this and get issues down the road. The same applies to adding the extra headers.
There was a problem hiding this comment.
That seems wise. Done!
| self.init() | ||
| self.reserveCapacity(headers.count) | ||
| // Don't add any pseudo-headers. | ||
| for (name, value, _) in headers where name.utf8.first != .some(UInt8(ascii: ":")) { |
There was a problem hiding this comment.
There's a mild perf improvement available here: well-formatted HTTP/2 header blocks must have all their pseudo-headers first. You can therefore do a dropFirst(where:) to only look at the minimal number of headers and then just do add(contentsOf:) or similar to reduce the amount of unnecessary checking you do.
There was a problem hiding this comment.
Oh, smart thinking, I like it!
Motivation: Following from grpc#1041, we'd like to switch our currency type on the server from HTTP/1 to HTTP/2. This means that to retain support for gRPC Web we need a codec to convert between HTTP/1 and HTTP/2 in the opposite direction to the codecs provided by NIO HTTP/2. Modifications: - Add a gRPC Web to HTTP2 codec Result: - Nothing yet, as this remains unused.
glbrntt
added
🔨 semver/patch
Motivation:
Following from #1041, we'd like to switch our currency type on the
server from HTTP/1 to HTTP/2. This means that to retain support for gRPC
Web we need a codec to convert between HTTP/1 and HTTP/2 in the opposite
direction to the codecs provided by NIO HTTP/2.
Modifications:
Result: