Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to v1.0.0-rc3 of the conformance suite #226

Merged
merged 2 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ BIN := .tmp/bin
CACHE := .tmp/cache
LICENSE_HEADER_YEAR_RANGE := 2022-2023
LICENSE_HEADER_VERSION := v1.28.1
CONFORMANCE_VERSION := v1.0.0-rc2
CONFORMANCE_VERSION := v1.0.0-rc3
PROTOC_VERSION ?= 25.3
GRADLE_ARGS ?=
PROTOC := $(BIN)/protoc
Expand Down Expand Up @@ -46,34 +46,34 @@ runconformance: runcrosstests runconformancenew
runconformancenew: generate $(CONNECT_CONFORMANCE) ## Run the new conformance test suite.
./gradlew $(GRADLE_ARGS) conformance:client:google-java:installDist conformance:client:google-javalite:installDist
$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/lite-unary-config.yaml \
--known-failing conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
conformance/client/google-javalite/build/install/google-javalite/bin/google-javalite \
--style suspend
$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/lite-unary-config.yaml \
--known-failing conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
conformance/client/google-javalite/build/install/google-javalite/bin/google-javalite \
--style callback
$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/lite-unary-config.yaml \
--known-failing conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
conformance/client/google-javalite/build/install/google-javalite/bin/google-javalite \
--style blocking
$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/standard-unary-config.yaml \
--known-failing conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
conformance/client/google-java/build/install/google-java/bin/google-java \
--style suspend
$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/standard-unary-config.yaml \
--known-failing conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
conformance/client/google-java/build/install/google-java/bin/google-java \
--style callback
$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/standard-unary-config.yaml \
--known-failing conformance/client/known-failing-unary-cases.txt -- \
--known-failing @conformance/client/known-failing-unary-cases.txt -- \
conformance/client/google-java/build/install/google-java/bin/google-java \
--style blocking
$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/lite-stream-config.yaml \
--known-failing conformance/client/known-failing-stream-cases.txt -- \
--known-failing @conformance/client/known-failing-stream-cases.txt -- \
conformance/client/google-javalite/build/install/google-javalite/bin/google-javalite
$(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/standard-stream-config.yaml \
--known-failing conformance/client/known-failing-stream-cases.txt -- \
--known-failing @conformance/client/known-failing-stream-cases.txt -- \
conformance/client/google-java/build/install/google-java/bin/google-java

.PHONY: runcrosstests
Expand Down Expand Up @@ -125,13 +125,20 @@ $(CONNECT_CONFORMANCE): $(CONNECT_CONFORMANCE_DOWNLOAD)

.PHONY: generate
generate: $(PROTOC) buildplugin generateconformance generateexamples ## Generate proto files for the entire project.
rm -rf protoc-gen-connect-kotlin/build/generated/sources/bufgen || true
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added these cleanup calls because we removed an unused message from the conformance protos, and it was causing a compile error until I did this due to an old, invalid source file being left around in the output folder.

buf generate --template protoc-gen-connect-kotlin/buf.gen.yaml -o protoc-gen-connect-kotlin protoc-gen-connect-kotlin/proto
rm -rf extensions/google-java/build/generated/sources/bufgen || true
rm -rf extensions/google-javalite/build/generated/sources/bufgen || true
buf generate --template extensions/buf.gen.yaml -o extensions buf.build/googleapis/googleapis
make licenseheaders

.PHONY: generateconformance
generateconformance: $(PROTOC) buildplugin ## Generate protofiles for conformance tests.
rm -rf conformance/google-java/build/generated/sources/bufgen || true
rm -rf conformance/google-javalite/build/generated/sources/bufgen || true
buf generate --template conformance/buf.gen.yaml -o conformance conformance/proto
rm -rf conformance/client/google-java/build/generated/sources/bufgen || true
rm -rf conformance/client/google-javalite/build/generated/sources/bufgen || true
buf generate --template conformance/buf.gen.yaml -o conformance/client buf.build/connectrpc/conformance:$(CONFORMANCE_VERSION)

.PHONY: generateexamples
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import com.connectrpc.conformance.v1.ClientCompatRequest.Cancel.CancelTimingCase
import com.connectrpc.conformance.v1.ClientErrorResult
import com.connectrpc.conformance.v1.ClientResponseResult
import com.connectrpc.conformance.v1.ClientStreamResponse
import com.connectrpc.conformance.v1.Code
import com.connectrpc.conformance.v1.Codec
import com.connectrpc.conformance.v1.Compression
import com.connectrpc.conformance.v1.ConfigProto
Expand Down Expand Up @@ -137,7 +138,7 @@ class JavaHelpers {

private fun toProtoError(ex: ConnectException): Error {
return Error.newBuilder()
.setCode(ex.code.value)
.setCode(toProtoCode(ex.code))
.setMessage(ex.message ?: ex.code.codeName)
.addAllDetails(
ex.details.map {
Expand All @@ -150,6 +151,10 @@ class JavaHelpers {
.build()
}

private fun toProtoCode(code: com.connectrpc.Code): Code {
return Code.forNumber(code.value) ?: Code.CODE_UNKNOWN
}

private fun toTypeUrl(typeName: String): String {
return if (typeName.contains('/')) typeName else TYPE_URL_PREFIX + typeName
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import com.connectrpc.conformance.v1.ClientCompatRequest.Cancel.CancelTimingCase
import com.connectrpc.conformance.v1.ClientErrorResult
import com.connectrpc.conformance.v1.ClientResponseResult
import com.connectrpc.conformance.v1.ClientStreamResponse
import com.connectrpc.conformance.v1.Code
import com.connectrpc.conformance.v1.Codec
import com.connectrpc.conformance.v1.Compression
import com.connectrpc.conformance.v1.ConformancePayload
Expand Down Expand Up @@ -129,7 +130,7 @@ class JavaLiteHelpers {

private fun toProtoError(ex: ConnectException): Error {
return Error.newBuilder()
.setCode(ex.code.value)
.setCode(toProtoCode(ex.code))
.setMessage(ex.message ?: ex.code.codeName)
.addAllDetails(
ex.details.map {
Expand All @@ -142,6 +143,10 @@ class JavaLiteHelpers {
.build()
}

private fun toProtoCode(code: com.connectrpc.Code): Code {
return Code.forNumber(code.value) ?: Code.CODE_UNKNOWN
}

private fun toTypeUrl(typeName: String): String {
return if (typeName.contains('/')) typeName else TYPE_URL_PREFIX + typeName
}
Expand Down
9 changes: 7 additions & 2 deletions conformance/client/known-failing-stream-cases.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,10 @@
# RPC deadlines, but that is not enforced when the request
# body is duplex. So timeouts don't currently work with
# bidi streams.
Timeouts/HTTPVersion:2/**/bidi half duplex timeout
Timeouts/HTTPVersion:2/**/bidi full duplex timeout
Timeouts/HTTPVersion:2/**/bidi-stream/**

# Deadline headers are not currently set.
Deadline Propagation/**

# Bug: incorrect code attribution for these failures (UNKNOWN instead of INTERNAL)
Connect Unexpected Responses/**/unexpected-stream-codec
35 changes: 32 additions & 3 deletions conformance/client/known-failing-unary-cases.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,32 @@
# Test runner is overly strict in asserting the contents
# of the message query param, so these tests currently fail.
Idempotency/**
# If error body is JSON null, it is interpreted as an unknown error
# instead of falling back to basing code on the HTTP status.
Connect Error and End-Stream/**/error/null

# Deadline headers are not currently set.
Deadline Propagation/**

# This response doesn't look like a normal Connect response, so it
# goes through okhttp's default 408 code handling, which retries.
# The retry triggers an error:
# client sent another request (#2) for the same test case
HTTP to Connect Code Mapping/**/request-timeout

# Bug: response content-type is not correctly checked
Unexpected Responses/**/unexpected-content-type

# Bug: "trailers-only" responses are not correctly identified.
# If headers contain "grpc-status", this client assumes it is a
# trailers-only response. However, a trailers-only response should
# instead be identified by lack of body or HTTP trailers.
gRPC Unexpected Responses/**/trailers-only/*

# Bug: if gRPC unary response contains zero messages or
# more than one message, client does not complain if
# status trailers says "ok"
gRPC Unexpected Responses/**/unary-multiple-responses
gRPC Unexpected Responses/**/unary-ok-but-no-response
gRPC-Web Unexpected Responses/**/unary-ok-but-no-response

# Bug: incorrect code attribution for these failures (INTERNAL instead of UNKNOWN)
gRPC-Web Unexpected Responses/**/missing-status
gRPC-Web Unexpected Responses/**/trailers-in-body/unary-multiple-responses
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@

package com.connectrpc.conformance.client

import com.connectrpc.Code
import com.connectrpc.ConnectException
import com.connectrpc.Headers
import com.connectrpc.ProtocolClientConfig
import com.connectrpc.RequestCompression
import com.connectrpc.ResponseMessage
import com.connectrpc.SerializationStrategy
import com.connectrpc.asConnectException
import com.connectrpc.compression.GzipCompressionPool
import com.connectrpc.conformance.client.adapt.AnyMessage
import com.connectrpc.conformance.client.adapt.BidiStreamClient
Expand Down Expand Up @@ -228,17 +228,8 @@ class Client(
}
} catch (ex: Throwable) {
if (!sent) {
val connEx = if (ex is ConnectException) {
ex
} else {
ConnectException(
code = Code.UNKNOWN,
message = ex.message,
exception = ex,
)
}
return ClientResponseResult(
error = connEx,
error = asConnectException(ex),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consolidated a few if (ex is ConnectException) ... blocks with a new helper function.

numUnsentRequests = 1,
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,9 @@
package com.connectrpc.conformance.client.adapt

import com.connectrpc.ClientOnlyStreamInterface
import com.connectrpc.Code
import com.connectrpc.ConnectException
import com.connectrpc.Headers
import com.connectrpc.ResponseMessage
import com.connectrpc.asConnectException
import com.google.protobuf.MessageLite
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.channels.ReceiveChannel
Expand Down Expand Up @@ -99,16 +98,12 @@ abstract class ClientStreamClient<Req : MessageLite, Resp : MessageLite>(
headers = underlying.responseHeaders().await(),
trailers = underlying.responseTrailers().await(),
)
} catch (e: Exception) {
val connectException = if (e is ConnectException) {
e
} else {
ConnectException(code = Code.UNKNOWN, exception = e)
}
} catch (ex: Exception) {
val connEx = asConnectException(ex)
return ResponseMessage.Failure(
cause = connectException,
cause = connEx,
headers = underlying.responseHeaders().await(),
trailers = connectException.metadata,
trailers = connEx.metadata,
)
}
}
Expand Down
13 changes: 13 additions & 0 deletions library/src/main/kotlin/com/connectrpc/ConnectException.kt
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,16 @@ data class ConnectException(
)
}
}

/**
* Returns a ConnectException for the given cause. If ex is a ConnectException
* then it is returned. Otherwise, it is wrapped in a ConnectException with
* the given code.
*/
fun asConnectException(ex: Throwable, code: Code = Code.UNKNOWN): ConnectException {
return if (ex is ConnectException) {
ex
} else {
ConnectException(code = code, exception = ex)
}
}
40 changes: 29 additions & 11 deletions library/src/main/kotlin/com/connectrpc/impl/ProtocolClient.kt
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ import com.connectrpc.ServerOnlyStreamInterface
import com.connectrpc.StreamResult
import com.connectrpc.StreamType
import com.connectrpc.UnaryBlockingCall
import com.connectrpc.asConnectException
import com.connectrpc.http.Cancelable
import com.connectrpc.http.HTTPClientInterface
import com.connectrpc.http.HTTPRequest
import com.connectrpc.http.HTTPResponse
import com.connectrpc.http.UnaryHTTPRequest
import com.connectrpc.http.dispatchIn
import com.connectrpc.http.transform
Expand Down Expand Up @@ -94,7 +96,20 @@ class ProtocolClient(
val unaryFunc = config.createInterceptorChain()
val finalRequest = unaryFunc.requestFunction(unaryRequest)
val cancelable = httpClient.unary(finalRequest) httpClientUnary@{ httpResponse ->
val finalResponse = unaryFunc.responseFunction(httpResponse)
val finalResponse: HTTPResponse
try {
finalResponse = unaryFunc.responseFunction(httpResponse)
} catch (ex: Throwable) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was one of the previously uncaught exceptions. It was happening when there was a problem in the wire format of the 5-byte envelopes around stream messages. Instead of being turned into an RPC failure, it was causing an okhttp worker thread to die, which would then cause the conformance client to hang and timeout.

val connEx = asConnectException(ex)
onResult(
ResponseMessage.Failure(
connEx,
emptyMap(),
connEx.metadata,
),
)
return@httpClientUnary
}
val exception = finalResponse.cause?.setErrorParser(serializationStrategy.errorDetailParser())
if (exception != null) {
onResult(
Expand All @@ -110,10 +125,10 @@ class ProtocolClient(
val responseMessage: Output
try {
responseMessage = responseCodec.deserialize(finalResponse.message)
} catch (e: Exception) {
} catch (ex: Exception) {
onResult(
ResponseMessage.Failure(
ConnectException(code = Code.INTERNAL_ERROR, exception = e),
asConnectException(ex, Code.INTERNAL_ERROR),
finalResponse.headers,
finalResponse.trailers,
),
Expand All @@ -129,8 +144,16 @@ class ProtocolClient(
)
}
return cancelable
} catch (e: Exception) {
throw e
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the other uncaught exception. 🤦
Since this is an async operation, instead of throwing, it needs to invoke the callback with an error.

} catch (ex: Exception) {
val connEx = asConnectException(ex)
onResult(
ResponseMessage.Failure(
connEx,
emptyMap(),
connEx.metadata,
),
)
return { }
}
}

Expand Down Expand Up @@ -228,12 +251,7 @@ class ProtocolClient(
try {
streamResult = streamFunc.streamResultFunction(initialResult)
} catch (ex: Throwable) {
val connEx = if (ex is ConnectException) {
ex
} else {
ConnectException(code = Code.UNKNOWN, exception = ex)
}
streamResult = StreamResult.Complete(connEx)
streamResult = StreamResult.Complete(asConnectException(ex))
}
when (streamResult) {
is StreamResult.Headers -> {
Expand Down
7 changes: 2 additions & 5 deletions okhttp/src/main/kotlin/com/connectrpc/okhttp/OkHttpStream.kt
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package com.connectrpc.okhttp
import com.connectrpc.Code
import com.connectrpc.ConnectException
import com.connectrpc.StreamResult
import com.connectrpc.asConnectException
import com.connectrpc.http.HTTPMethod
import com.connectrpc.http.HTTPRequest
import com.connectrpc.http.Stream
Expand Down Expand Up @@ -134,11 +135,7 @@ private class ResponseCallback(
// This is the final chance to notify trailers to the consumer.
val connectEx = when (exception) {
null -> null
is ConnectException -> exception
else -> ConnectException(
code = codeFromException(call.isCanceled(), exception),
exception = exception,
)
else -> asConnectException(exception, codeFromException(call.isCanceled(), exception))
}
val finalResult = StreamResult.Complete<Buffer>(
trailers = response.safeTrailers() ?: emptyMap(),
Expand Down
Loading