Resolve all outstanding conformance opt-outs #271
Conversation
| /// The complexity around configuring callbacks on this type is an artifact of the library | ||
| /// supporting both callbacks and async/await. This is internal to the package, and not public. | ||
| final class ClientOnlyStream<Input: ProtobufMessage, Output: ProtobufMessage>: @unchecked Sendable { |
There was a problem hiding this comment.
I'm not thrilled about the necessary callback configurations in this class, but it's fairly similar to what's present in BidirectionalAsyncStream. The underlying reason is that since we support both classic closures/callbacks and async/await, everything is currently backed by closures/callbacks (with async/await layered on top in separate classes that are available on iOS 13+) which need to be instantiated in a specific order to work with URLSession. If we bump our minimum SDK version and move away from callbacks at some point or invert this structure, we can simplify these
| /// far, including the one being validated. | ||
| /// | ||
| /// - returns: The validated stream result, which may have been transformed into an error. | ||
| func validatedForClientStream(receivedMessageCount: Int) -> Self { |
There was a problem hiding this comment.
For any Kotlin-focused code reviewers 😄 Swift defaults to internal so any unannotated functions are not exposed from the package
rebello95
force-pushed
the
rebello-conformance-fixes
branch
from
cf171fa
to
3118c1a
Compare
| error: response.error, | ||
| // If content-type looks like it could be an RPC server's response, consider | ||
| // this an internal error. | ||
| let code: Code = contentType.hasPrefix("application/") ? .internalError : .unknown |
There was a problem hiding this comment.
Is a non-200 HTTP status handled somewhere else? I expected to see it handled just above, or the HTTP status incorporated here (so deriving a code from the HTTP status instead of hard-coding to "unknown").
There was a problem hiding this comment.
The non-200 cases are falling into one of the else/else if cases below depending on whether the error response is compressed (Connect Compressed Error and End-Stream/HTTPVersion:2/TLS:false/error/compressed is one of these cases). Both of these use response.code, which is the Connect code from the HTTP status.
| let receivedMessageCount = self.receivedMessageCount.perform { value in | ||
| if case .message = result { | ||
| value += 1 | ||
| } | ||
| return value | ||
| } |
There was a problem hiding this comment.
I'm not really following this block. I think it means that receive must be called 2x in order to see the count bump to 2. Is that right? If so, what if the caller only calls receive once? It's not clear to me how we are actually validating that there isn't more than one message in the response, other than returning an error if there is when the application asks for another.
There was a problem hiding this comment.
This is related to the wiring I alluded to in my earlier comments - receive is an internal function called by the protocol client to pass a network chunk to this handle. It's not something called by end-users, it's called by internals to pass that result on to end users. I'll rename these to try to provide a bit more clarity (and I'd really like to remove some of this in the future if we can centralize on async/await at some point). Let me know if that clears this up
| /// Close the stream and await a response. | ||
| /// No calls to `send()` are valid after calling `closeAndReceive()`. | ||
| func closeAndReceive() |
There was a problem hiding this comment.
In other Connect and gRPC operations, I'm used to seeing this atomically fuse the close and receive operations, so that this method returns the one and only response message (or an error). So this seems a little confusingly named since the caller still must call receive right after. Or am I misunderstanding this?
There was a problem hiding this comment.
Related to #271 (comment), there is no receive function on this interface (that's a function on the internal concrete implementation for wiring), and consumers do not manually fetch results from the stream. Consumers observe responses/results through the results() async stream, such as by doing for await result in stream.results() { ... } or let result = await stream.results.take(1)
There was a problem hiding this comment.
Alternatively, I am open to different naming suggestions. Also not opposed to changing this function to be async, removing results() from ClientOnlyAsyncStreamInterface, and returning the result here. @eseay thoughts?
There was a problem hiding this comment.
Consumers observe responses/results through the
results()async stream
If the server sends two messages for a client-stream operation, instead of only one, does the async stream send the first message to the application and then send an error on receipt of the second message? If so, I think the appropriate behavior is to only deliver the message after trailers are received, so you know it was an OK status and exactly one message. That way the application only sees a response message for a well-formed response stream.
Maybe that's what already happens. Sorry if I'm making comments/nitpicks without actually having a clear understanding of what this is doing.
There was a problem hiding this comment.
If the server sends two messages for a client-stream operation, instead of only one, does the async stream send the first message to the application and then send an error on receipt of the second message?
Yes, that is what happens now.
If so, I think the appropriate behavior is to only deliver the message after trailers are received, so you know it was an OK status and exactly one message. That way the application only sees a response message for a well-formed response stream.
To verify I'm understanding correctly, you're suggesting the client pass headers but buffer the body until trailers are received, then verify that only one message was received and return only an error (and no messages) if multiple messages are present? If so, is this something the conformance tests can validate?
There was a problem hiding this comment.
If so, is this something the conformance tests can validate?
Supposedly it is. It should be complaining if it sees a response message in the ClientCompatResponse but the test wasn't expecting one. Is it possible your conformance client is ignoring the initial received message when it subsequently encounters an error?
The logic here should complain with a message like so:
expecting 0 response messages but instead got 1
There was a problem hiding this comment.
Interesting. I added some logging and this is what I see when running the Connect Unexpected Responses/HTTPVersion:1/TLS:false/client-stream/multiple-responses test case:
Running 1 tests with reference server for server config {HTTP_VERSION_1, PROTOCOL_CONNECT, TLS:false}...
Received: headers(["content-type": ["application/connect+proto"], "content-length": ["65"]])
Converted into: headers(["content-type": ["application/connect+proto"], "content-length": ["65"]])
URLSession received 65 data bytes
Connect interceptor got 5 data bytes
Connect interceptor not passing message because it is empty
Connect interceptor got 5 data bytes
Connect interceptor not passing message because it is empty
Connect interceptor got 55 data bytes
Connect interceptor received end stream with 50 end stream response bytes
Received: complete(code: Connect.Code.ok, error: nil, trailers: nil)
Converted into: complete(code: Connect.Code.internalError, error: Optional(Connect.ConnectError(code: Connect.Code.unimplemented, message: Optional("unary stream has no messages"), exception: nil, details: [], metadata: [:])), trailers: nil)
Total cases: 1
1 passed, 0 failed
So I think what's happening is the Connect/gRPC/gRPC-Web interceptors are are stripping empty messages rather than passing them through to the end client (here, for example) and the conformance suite is sending empty messages, so you're right that no message is being returned to the conformance runner. In fact, this is actually falling into the unary stream has no messages case because none of the empty messages are being passed through, and the test is passing because both happen to require the same status code.
What should the behavior be in this case? Should the client generally be passing empty messages through?
There was a problem hiding this comment.
Yes, an empty message is a valid message as long as the codec accepts an empty bytes input. If the codec does not accept the empty bytes, then you'd report the error to the client like any other error thrown by the decoding (like from a malformed paylad).
There was a problem hiding this comment.
Thanks for confirming. I pushed 2 commits:
The first one gets these two tests to fail by no longer stripping empty responses:
FAILED: Connect Unexpected Responses/HTTPVersion:1/TLS:false/client-stream/multiple-responsesFAILED: gRPC-Web Unexpected Responses/HTTPVersion:1/TLS:false/trailers-in-body/client-stream/multiple-responses
The second commit applies buffering to wait for client streams to complete before doing validation and then passing the results to the caller.
Signed-off-by: Michael Rebello <me@michaelrebello.com>
rebello95
force-pushed
the
rebello-conformance-fixes
branch
from
073cff2
to
c308226
Compare
|
Thank you for the detailed review @jhump! 🙏🏽 |
This PR includes updates to fix all outstanding conformance test failures and removes the corresponding opt-outs. All changes are validated by the conformance test suite.
Many of these fixes are similar to connectrpc/connect-kotlin#248 and connectrpc/connect-kotlin#274.
Resolves #268.
Resolves #269.
Resolves #270.
This should also be one of the final changes before v1.0 #222.