Add GRPCStreamStateMachine #1787
Conversation
gjcairo
force-pushed
the
stream-state-machine
branch
from
fcae3f7 to
409afbb
Compare
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Just dropping my first set of comments. Will do the server state machine in a separate batch.
There was a problem hiding this comment.
This is taking shape. One thing that surprised me a little was having two separate state machines, as I figured they'd have more in common (there's nothing wrong with having two). Did you consider having a single state machine and switching on whether it's a client/server in the cases where the logic differs?
| case .clientClosedServerIdle, | ||
| .clientIdleServerIdle, | ||
| .clientOpenServerClosed, | ||
| .clientClosedServerClosed: | ||
| return nil |
There was a problem hiding this comment.
I think we need to try when the client is closed too
There was a problem hiding this comment.
You're right we should try on clientClosedServerClosed, but I don't think we should when clientClosedServerIdle, because we should first wait to be open.
gjcairo
force-pushed
the
stream-state-machine
branch
from
65c74e4 to
ea17532
Compare
gjcairo
force-pushed
the
stream-state-machine
branch
from
ea17532 to
96c01f1
Compare
gjcairo
force-pushed
the
stream-state-machine
branch
from
0edda8f to
01a0806
Compare
gjcairo
force-pushed
the
stream-state-machine
branch
2 times, most recently
from
d7d0bdb to
3df4239
Compare
gjcairo
force-pushed
the
stream-state-machine
branch
from
3df4239 to
3ba0597
Compare
FranzBusch
left a comment
FranzBusch
left a comment
There was a problem hiding this comment.
Overall looks great! Left some minor comments
|
|
||
| /// The value as a String. If it was originally stored as a binary, the base64-encoded String version | ||
| /// of the binary data will be returned instead. | ||
| public func encoded() -> String { |
There was a problem hiding this comment.
To align with other existing API https://developer.apple.com/documentation/foundation/data/2142853-base64encodedstring
| public func encoded() -> String { | |
| public func base64EncodedString() -> String { |
There was a problem hiding this comment.
It's only a base64 encoded if the underlying value is binary so the base64EncodedString isn't accurate.
| .flatMap { Int($0) } | ||
| .map { HTTPResponseStatus(statusCode: $0) } |
There was a problem hiding this comment.
NIT: Any reason to not write this in a single line?
There was a problem hiding this comment.
You mean why not .flatMap { HTTPResponseStatus(statusCode: Int($0)) }?
Int($0) can return nil so this way I avoid having to explicitly unwrap the optional.
| .flatMap { Int($0) } | ||
| .map { HTTPResponseStatus(statusCode: $0) } |
| } | ||
| } | ||
|
|
||
| private mutating func clientReceive(bytes: ByteBuffer, endStream: Bool) throws { |
There was a problem hiding this comment.
Why do some methods operate on [UInt8] and others on ByteBuffer?
There was a problem hiding this comment.
When we receive data, we receive ByteBuffers from the channel. However we then deal with [UInt8] internally, as we decode bytes into RPC messages, meaning the outbound methods operate on [UInt8].
| // See: | ||
| // - https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md | ||
| enum ContentType { | ||
| case protobuf |
There was a problem hiding this comment.
Is this naming from their docs because I find it confusing that it is protobuf but we send "application/grpc"
There was a problem hiding this comment.
I kept the same naming from v1. application/grpc and application/grpc+proto are interchangeable, so I think that's where the name comes from.
There was a problem hiding this comment.
Franz has a point though, grpc would be clearer here
| import NIOHPACK | ||
| import XCTest | ||
|
|
||
| @testable import GRPCHTTP2Core |
There was a problem hiding this comment.
Out of interest, could we do this without a @testable import? I always try to aim for not testing internals or use the package modifier since that allows to run tests in release mode
There was a problem hiding this comment.
I'm using the state enum, as well as the header key names which are all internal in the state machine - that's why the @testable is needed.
As for package, you mean using that instead of internal for the types that have to be used from tests?
There was a problem hiding this comment.
Yes exactly. The package modifier should be able to replace all @testable usage or @_spi(Testing) in the long run.
gjcairo
force-pushed
the
stream-state-machine
branch
from
3ba0597 to
41f3087
Compare
| // See: | ||
| // - https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md | ||
| enum ContentType { | ||
| case protobuf |
There was a problem hiding this comment.
Franz has a point though, grpc would be clearer here
Sources/GRPCHTTP2Core/Internal/GRPCStatusMessageMarshaller.swift
Outdated
Show resolved
Hide resolved
gjcairo
force-pushed
the
stream-state-machine
branch
3 times, most recently
from
c2dee60 to
7059146
Compare
gjcairo
force-pushed
the
stream-state-machine
branch
from
7059146 to
6e780cf
Compare
| // If status isn't present, it means we're returning a non-200 HTTP :status | ||
| // header. This should already be included in the custom metadata: assert | ||
| // this and simply return those headers. |
There was a problem hiding this comment.
I don't think this is so obvious and blurs the lines between the grpc and http layers.
Specifically, "metadata" is a gRPC concept, and ":status" is an http concept so it's confusing to pass the HTTP header in the gRPC metadata only to extract it and put it back in the HPACKHeaders in makeNon200StatusTrailers.
For non-200 cases we never reach the gRPC layer at all so we shouldn't be dealing with metadata.
I think we should be calling something like makeNon200StatusTrailers directly and without passing it Metadata where we know we need the non-200 response headers.
There was a problem hiding this comment.
Actually, I was never passing a nil Status in this method, meaning makeNon200StatusTrailers was never being called. This was a leftover from a previous change where I was building trailers differently.
We'll always have a grpc-status and a :status equal to 200 in all the cases we build trailers. The only scenario where we don't, is when we respond with an Unsupported Media Type HTTP status code (415) in serverReceive(metadata:endStream:configuration:). I can get rid of this code altogether.
gjcairo
force-pushed
the
stream-state-machine
branch
from
bfdd772 to
8fda194
Compare
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
This is getting very close to be ready to merge. I left a few nits/comments.
| mutating func send( | ||
| status: Status, | ||
| metadata: Metadata, | ||
| trailersOnly: Bool |
There was a problem hiding this comment.
Is trailersOnly necessary here? Whether it's trailers-only seems to me to be a function of the current state, rather than something the caller would determine.
There was a problem hiding this comment.
Yeah you're right, this was left over from before one of the many refactors and wasn't necessary anymore; I actually found a state transition bug removing it, so good catch.
gjcairo
force-pushed
the
stream-state-machine
branch
from
8fda194 to
9aa7705
Compare
gjcairo
force-pushed
the
stream-state-machine
branch
from
e083bfd to
61135de
Compare
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
I think this looks good, nice one Gus!
| headers.add(methodDescriptor.fullyQualifiedMethod, forKey: .path) | ||
| headers.add(scheme.rawValue, forKey: .scheme) | ||
|
|
||
| // The order is important here: reserved HTTP2 headers (those starting with `:`) |
There was a problem hiding this comment.
fyi they're called "pseudo-headers"
3 participants
Motivation
We need a state machine to drive the stream lifetime logic for both the client and the server.
Modifications
Added a GRPCStreamStateMachine that drives the logic for both clients and servers.
Result
Closer to having a complete HTTP2 transport implementation.