-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
…nt the new test cases that don't yet pass
@@ -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 |
There was a problem hiding this comment.
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.
return ClientResponseResult( | ||
error = connEx, | ||
error = asConnectException(ex), |
There was a problem hiding this comment.
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.
val finalResponse: HTTPResponse | ||
try { | ||
finalResponse = unaryFunc.responseFunction(httpResponse) | ||
} catch (ex: Throwable) { |
There was a problem hiding this comment.
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.
@@ -129,8 +144,16 @@ class ProtocolClient( | |||
) | |||
} | |||
return cancelable | |||
} catch (e: Exception) { | |||
throw e |
There was a problem hiding this comment.
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.
These new test cases found some uncaught-exception bugs that are fixed in here. It also found some other small bugs in handling anomalous/erroneous responses. These are not high priority, so instead of trying to fix all of them, I've documented them all in the known failing config files.
Here are relevant release notes for rc3: https://github.com/connectrpc/conformance/releases/tag/v1.0.0-rc3
There were some backwards-incompatible changes that required some small changes in the config and the conformance client code. (Once we hit v1.0.0 of the conformance suite, we'll consider it stable and avoid such breaking changes thereafter.)