Must use alternate model because of incompatibilities between java and javalite runtimes with well-known types #194
Conversation
| freeCompilerArgs += "-opt-in=kotlin.RequiresOptIn" | ||
| } | ||
| } | ||
| shadowJar { | ||
| archiveBaseName.set("shadow") | ||
| archiveFileName.set("conformance-client-java.jar") |
There was a problem hiding this comment.
I made this change to make it easier to actually use the output JAR from another target in the Makefile. Without this, the actual file has a <version>-SNAPSHOT in the filename. But I couldn't find any simple way to figure out the <version>, so that a command could reference the correct filename. (It appears to be computed dynamically by Gradle based on git tags.) So this removes the suffix so we can easily reference the correct output JAR file.
There was a problem hiding this comment.
Another option here is that you can make it use the application plugin (https://docs.gradle.org/current/userguide/application_plugin.html) which when combined with installDist will create an executable script with the right jar file name embedded. I do that in protoc-gen-connect-kotlin to make it callable.
There was a problem hiding this comment.
🤦, doh! I should have done that to begin with -- good idea! I wasn't aware of the application plugin and was mainly used to building "fat jars" when doing Java dev with Maven way back in the day. That's how I ended up finding the shadowjar plugin.
I've ripped out the shadowjar stuff and converted both of these conformance programs to use the application plugin. Thanks for the pointer!
| @@ -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-rc1 | |||
| CONFORMANCE_VERSION := v1.0.0-rc2 | |||
There was a problem hiding this comment.
The main change this brings in is a new method on the conformance service: IdempotentUnary, which is just like Unary, but it is declared to be side-effect-free so it can be used with Connect GET.
| @@ -49,14 +50,4 @@ class JavaInvoker( | |||
| override fun bidiStreamClient(): BidiStreamClient<*, *> { | |||
| return JavaBidiStreamClient(client) | |||
| } | |||
|
|
|||
| companion object { | |||
| fun serializationStrategy(codec: Codec): SerializationStrategy { | |||
There was a problem hiding this comment.
I moved this helper function into the new JavaHelpers, which contains other helper functions now that there are more things to adapt from the generated code to the model used by conformance/client.
| e.printStackTrace(System.err) | ||
| Runtime.getRuntime().exit(1) |
There was a problem hiding this comment.
Oops. You left a comment about this in the previous PR, but I mistakenly only applied it to the google-javalite/.../Main.kt file and not to this one. So correcting that here.
| // Clean-up HTTP client. | ||
| httpClient.connectionPool.evictAll() | ||
| httpClient.dispatcher.executorService.shutdown() |
There was a problem hiding this comment.
I made these changes so that the conformance client can cleanly shutdown without using System.exit. It turns out that the old code was creating lots of HTTP clients and never cleaning them up, which left non-daemon threads around, preventing the JVM from promptly terminating after the main function returned.
| client: UnaryClient<Req, Resp>, | ||
| requestType: String, |
There was a problem hiding this comment.
This parameter isn't used yet, but it will be needed since the lite runtime generated code doesn't actually know the protobuf names for generated types, and we need to verify that the request indicates the correct request type (since this method is re-used across all unary operations, so three different request message types: UnaryRequest, IdempotentUnaryRequest, and UnimplementedRequest).
| // okhttp *requires* that protocols contains HTTP_1_1 | ||
| // or H2_PRIOR_KNOWLEDGE. So we leave 1.1 in here, but | ||
| // expect HTTP/2 to always be used in practice since it | ||
| // should be negotiated during TLS handshake, | ||
| listOf(okhttp3.Protocol.HTTP_2, okhttp3.Protocol.HTTP_1_1) |
There was a problem hiding this comment.
The previous formulation could return a set of protocols that only included HTTP_2. However, okhttp3 doesn't like this and expects HTTP_1_1 or H2_PRIOR_KNOWLEDGE to also be in the list, just in case it encounters a non-TLS request URL.
…d javalite runtimes with the wellknown types
jhump
force-pushed
the
jh/overhaul-conformance-client
branch
from
dcd11b6
to
a76bbd7
Compare
There was a problem hiding this comment.
Overall looks good. I like that we're testing both lite and full runtimes, but it feels pretty complex to go through all the hoops. I guess another alternative would be we could just run all the tests on the lite runtime (since the full runtime is additive) and just write some selected unit tests for the full runtime classes to verify serialization/deserialization.
I'm ok with this approach just wanted to throw out another option if this gets too tedious.
| freeCompilerArgs += "-opt-in=kotlin.RequiresOptIn" | ||
| } | ||
| } | ||
| shadowJar { | ||
| archiveBaseName.set("shadow") | ||
| archiveFileName.set("conformance-client-java.jar") |
There was a problem hiding this comment.
Another option here is that you can make it use the application plugin (https://docs.gradle.org/current/userguide/application_plugin.html) which when combined with installDist will create an executable script with the right jar file name embedded. I do that in protoc-gen-connect-kotlin to make it callable.
The JSON format is not supported in the lite runtime, so I think it's worth running the whole conformance suite against both runtimes just to make sure we're running the tests using JSON. But I suppose I could be convinced the other way. How about we see how it goes maintaining both, and if maintenance/fixes/etc turn out to be's a pain, we can change our mind and simplify? |
Refresher: the approach made use of lite and non-lite generated code in the same process (generated to different packages), so that the core client could use the lite messages, and the non-lite tests would just swap in the non-lite runtime and then do runtime-conversions to the non-lite message types (by serializing and then de-serializing).
I thought I had done enough with the previous formulation to vet that the approach would work. It compiled fine and even ran fine -- it parsed the first request from the test runner, but then blew up (as expected) from the
TODOcall inClient.kt.Sadly, that was not enough. Trying to unmarshal later test cases would blow up with a class verifier error(!!!). This is because of the way classes are lazy-loaded in the JVM. So it only loaded the
ClientCompatResponse.Cancelmessage later, when a request actually used the relevant field. And that class, generated for the lite runtime, has an incompatibility between the lite and non-lite runtimes 😢. The reason is because this message refers togoogle.protobuf.Empty, which extendsgoogle.protobuf.GeneratedMessageLitein the lite runtime butGeneratedMessageV3in the non-lite runtime. Some of the generated code inCancelcompiles to a virtual call to a method ofEmptythat is defined on theGeneratedMessageLitesuper-class in the lite runtime. This results in aninvokevirtualinstruction in the byte code that indicates a method ofGeneratedMessageLite, and that is passingEmptyas the receiver. When loading that class with the non-lite runtime, the verifier sees that this is invalid becauseEmptydoes not extendGeneratedMessageLite. 🤦So... back to the drawing board. I've now manually created POJOs for the request and relevant sub-messages, to which the lite and non-lite runtimes can adapt their generated messages. So the generated code for the lite runtime moves out of
conformance/clientand intoconformance/client/google-javalite, sinceconformance/clientnow just refers to its own POJO models.Unfortunately, this means more code -- both to define the POJOs and to implement conversion from the generated Protobuf types to those POJOs, which is practically the same code, but just referencing different types (lite vs. non-lite generated types). Still better than everything being forked across the two runtimes, but not as concise as I was hoping it could be.