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.

Sign up for GitHub

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

Jump to bottom

Implement stream operations in the conformance client #196

Merged
merged 7 commits into from
Jan 16, 2024
Merged

Implement stream operations in the conformance client #196

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9f82a3d
get client stream operations working -- all but 'client max message s…
jhump Jan 11, 2024
0df45eb
get server stream operations working - some strange errors with grpc …
jhump Jan 12, 2024
798285b
get half-duplex bidi stream operations working - timeouts aren't work…
jhump Jan 12, 2024
3758032
get full-duplex bidi stream operations working - timeouts not working…
jhump Jan 12, 2024
214c1c5
ready to merge to main
jhump Jan 12, 2024
ea06627
spotless apply
jhump Jan 12, 2024
ca1b463
add TODO about TLS client certs
jhump Jan 16, 2024
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
23 changes: 16 additions & 7 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,30 +46,39 @@ 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-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-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-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-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-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-cases.txt -- \
--known-failing conformance/client/known-failing-unary-cases.txt -- \
conformance/client/google-java/build/install/google-java/bin/google-java \
--style blocking
# TODO: streaming conformance test cases

# TODO: Add streaming conformance tests. Currently, a small number of the test cases
# are flaky, so leaving this commented out for now.
# (Will continue investigating and address soon).
# $(CONNECT_CONFORMANCE) -v --mode client --conf conformance/client/lite-stream-config.yaml \
# --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 -- \
# conformance/client/google-java/build/install/google-java/bin/google-java

.PHONY: runcrosstests
runcrosstests: generate ## Run the old cross-test suite.
Expand Down
2 changes: 2 additions & 0 deletions .../client/google-java/src/main/kotlin/com/connectrpc/conformance/client/java/JavaHelpers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,8 @@ class JavaHelpers {
get() = msg.serverTlsCert
override val clientTlsCreds: TlsCreds?
get() = if (msg.hasClientTlsCreds()) TlsCredsImpl(msg.clientTlsCreds) else null
override val receiveLimitBytes: Int
get() = msg.messageReceiveLimit
override val timeoutMs: Int
get() = msg.timeoutMs
override val requestDelayMs: Int
Expand Down
2 changes: 2 additions & 0 deletions ...le-javalite/src/main/kotlin/com/connectrpc/conformance/client/javalite/JavaLiteHelpers.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ class JavaLiteHelpers {
get() = msg.serverTlsCert
override val clientTlsCreds: TlsCreds?
get() = if (msg.hasClientTlsCreds()) TlsCredsImpl(msg.clientTlsCreds) else null
override val receiveLimitBytes: Int
get() = msg.messageReceiveLimit
override val timeoutMs: Int
get() = msg.timeoutMs
override val requestDelayMs: Int
Expand Down
15 changes: 15 additions & 0 deletions conformance/client/known-failing-stream-cases.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# OkHttp seems to have a bug where timeout is not properly
# enforced when request body is full-duplex.
Timeouts/HTTPVersion:2/**/bidi half duplex timeout
Timeouts/HTTPVersion:2/**/bidi full duplex timeout

# Connect-kotlin does not have a way to limit the size of messages
# received. It probably should. Despite this, many cases in this suite
# still pass, so they are likely not exercising what we think they are.
# TODO: add flag to config yaml for whether implementation supports
# a receive size limit
Client Message Size/**/Compression:COMPRESSION_GZIP/TLS:false/**/client stream first request exceeds client limit
Client Message Size/**/Compression:COMPRESSION_GZIP/TLS:false/**/client stream subsequent request exceeds client limit
Client Message Size/**/Compression:COMPRESSION_GZIP/TLS:false/**/client stream all requests equal to client limit
Client Message Size/**/Compression:COMPRESSION_GZIP/TLS:false/**/server stream request equal to client limit
Client Message Size/**/Compression:COMPRESSION_GZIP/TLS:false/**/server stream request exceeds client limit
0 conformance/client/known-failing-cases.txt → ...ance/client/known-failing-unary-cases.txt
View file
Open in desktop
File renamed without changes.
23 changes: 23 additions & 0 deletions conformance/client/lite-stream-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# This configures the features that this client
# supports and that will be verified by the
# conformance test suite.
features:
versions:
- HTTP_VERSION_1
- HTTP_VERSION_2
protocols:
- PROTOCOL_CONNECT
- PROTOCOL_GRPC
- PROTOCOL_GRPC_WEB
codecs:
- CODEC_PROTO
# Lite does not support JSON
compressions:
- COMPRESSION_IDENTITY
- COMPRESSION_GZIP
streamTypes:
# This config file only runs stream RPC test cases.
- STREAM_TYPE_CLIENT_STREAM
- STREAM_TYPE_SERVER_STREAM
- STREAM_TYPE_HALF_DUPLEX_BIDI_STREAM
- STREAM_TYPE_FULL_DUPLEX_BIDI_STREAM
2 changes: 0 additions & 2 deletions conformance/client/lite-unary-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,3 @@ features:
# so that we can run them all three ways: suspend,
# callback, and blocking.
- STREAM_TYPE_UNARY
supportsTlsClientCerts: true
supportsHalfDuplexBidiOverHttp1: true
Comment on lines -23 to -24
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've removed the line about client certs since this property defaults to false. An error is occurring when this code tries to instantiate a client cert from the data in the conformance request. I haven't dug into it, and decided to just omit it for now.

It turns out that we actually can't do half-duplex stuff over HTTP 1.1, because we don't actually know ahead of time, for a given bidi RPC, if it's half-duplex or full-duplex (in the framework, based on generated protobuf metadata). But the request body must be marked as "duplex" if it might be full-duplex. So have to mark the body as duplex for all bidi RPCs, and okhttp then disallows them to be used with HTTP 1.1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to comment these properties out with a TODO to know to come back to them instead of removing them? Up to you but might help identify remaining work since it sounds like there are several things to track down.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I think we can leave the supportsHalfDuplexBidiOverHttp1 out since there's no realistic way of changing that. But, yes, I should add back supportsTlsClientCerts with a TODO to get them working.

Loading
Loading