Add Server support #447

luqasn · 2021-04-10T09:19:30Z

Excited to have this in a shareable state and very interested in hearing your feedback :)

I created separate commits for refactorings that were required for the new code that then gets added "append-only" in one larger commit to make it easier to gauge the impact (rather non-impact) on the existing code.

One thing that is not perfect yet is that the synthetic arg and result structs are generated on the same level as the normal ones, opening the possibility for a name clash. Ideally, I would want them nested inside the ServerCall class, but that required changes to the existing data class/union generators, so I skipped it for now.

codecov-io · 2021-04-10T09:23:44Z

Codecov Report

Merging #447 (5dfdc01) into master (27438c5) will increase coverage by 0.89%.
The diff coverage is 67.76%.

@@             Coverage Diff              @@
##             master     #447      +/-   ##
============================================
+ Coverage     59.53%   60.42%   +0.89%     
- Complexity      796      847      +51     
============================================
  Files            60       61       +1     
  Lines          5535     6198     +663     
  Branches        876      981     +105     
============================================
+ Hits           3295     3745     +450     
- Misses         1996     2153     +157     
- Partials        244      300      +56

Impacted Files	Coverage Δ	Complexity Δ
.../com/microsoft/thrifty/compiler/ThriftyCompiler.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...n/com/microsoft/thrifty/gradle/ThriftyExtension.kt	`20.56% <0.00%> (-0.20%)`	`6.00 <0.00> (ø)`
...kotlin/com/microsoft/thrifty/gradle/ThriftyTask.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...java/com/microsoft/thrifty/testing/TestClient.java	`60.46% <60.46%> (ø)`	`39.00 <39.00> (?)`
.../com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt	`83.56% <96.15%> (+1.21%)`	`159.00 <7.00> (+8.00)`
...tlin/com/microsoft/thrifty/schema/ServiceMethod.kt	`67.60% <100.00%> (+11.00%)`	`20.00 <2.00> (+2.00)`
.../kotlin/com/microsoft/thrifty/schema/StructType.kt	`44.00% <0.00%> (ø)`	`20.00% <0.00%> (+1.00%)`
...ain/kotlin/com/microsoft/thrifty/kgen/TypeUtils.kt	`89.83% <0.00%> (+1.69%)`	`0.00% <0.00%> (ø%)`
...kotlin/com/microsoft/thrifty/schema/ServiceType.kt	`62.50% <0.00%> (+1.78%)`	`18.00% <0.00%> (+1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27438c5...5dfdc01. Read the comment docs.

luqasn · 2021-04-10T10:57:28Z

One other thing that is a bit suboptimal is that the default error handler does not log anything when an error occurs. I didn't add that because up until now there was no dependency on any logging framework in thrifty-runtime and I was hesitant to add one. But it might be worth it, what do you think?

luqasn · 2021-04-11T08:47:02Z

Side note: I am pretty confident that apart from the default requiredness thing (which is very minor), Thrifty is 1 to 1 compatible/equivalent with the thrift implementation.
You cited the lack of isSet* properties as a source of difference and non-conformity making Thrifty unfit for the server side, but I am pretty sure that's only the case if you were to use non-boxed primitives (as is the case in default Java thrift), as the isSet* implementation for reference types is already just a null check on the field in default Java thrift.
But with Kotlin, the primitives are boxed anyways (at least if optional, not sure about non-optional), so a null check is always equivalent to an isSet call.

luqasn · 2021-05-21T08:29:14Z

Friendly nudge ;)

benjamin-bader

Hey, I'm very sorry for the delay. This honestly just fell off my radar after a very hectic month at work.

This is a great piece of work! I quibble about some minor things, but not enough to block on. I'd like to get this landed and refine as needed.

benjamin-bader · 2021-04-12T15:44:13Z

thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt


    fun generate(schema: Schema): List<FileSpec> {
-        val specsByNamespace = LinkedHashMultimap.create<String, TypeSpec>()


If we promote these to instance variables, let's clear them at the start of generate. If nothing else, it may make test failures a bit less confusing if we were to ever re-use an instance of KotlinCodeGenerator.

benjamin-bader · 2021-05-24T06:09:14Z

...ion-tests/src/test/kotlin/com/microsoft/thrifty/integration/conformance/server/TestServer.kt

+import okio.Buffer
+
+
+class TestServer @JvmOverloads constructor(private val protocol: ServerProtocol = ServerProtocol.BINARY) : Extension, BeforeEachCallback, AfterEachCallback {


Curious at the use of @JvmOverloads. Do you expect this to be used from Java?

This file was based on the existing TestServer.java that I converted to Kotlin using IntelliJ, so it was so helpful as to automatically add the annotation :)
I've removed it now.

benjamin-bader · 2021-05-24T06:15:56Z

...ts/src/test/kotlin/com/microsoft/thrifty/integration/conformance/server/ThriftTestHandler.kt

+     *     2 => { 6 => <empty Insanity struct>, },
+     *   }
+     */
+        val result: MutableMap<Long, Map<com.microsoft.thrifty.integration.kgen.coro.Numberz, com.microsoft.thrifty.integration.kgen.coro.Insanity>> = LinkedHashMap()


If possible can we import these types?

benjamin-bader · 2021-05-24T06:16:27Z

thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt

@@ -62,6 +62,7 @@ import com.microsoft.thrifty.service.AsyncClientBase
 import com.microsoft.thrifty.service.MethodCall
 import com.microsoft.thrifty.service.ServiceMethodCallback
 import com.microsoft.thrifty.service.TMessageType
+import com.microsoft.thrifty.service.server.*


nit: we generally try to avoid wildcard imports.

benjamin-bader · 2021-05-24T06:17:38Z

thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt

@@ -311,6 +317,21 @@ class KotlinCodeGenerator(
            }
        }

+        if (generateServer) {
+            check(coroutineServiceClients) { "Server feature currently requires coroutineServiceClients to be enabled" }


Have you considered having the generate do either clients or servers?

Yeah, good point. I wanted to add as little stuff as possible and thus thought it would be a good idea to reuse the client interface as the service interface (server side), because the signatures are exactly going to match.
But because it did not make much sense to me to implement the callback based variant for the server side, I chose to force the coroutine client flag so that the interface would be generated with coroutine suspend keywords and that's what the server generator will generated against.
It is kinda hacky though and you are totally right, I would suspect you either generate the client or the server, but rarely both at the same time.
For this reason and also because of the weird interaction between client and server implementations, it would make more sense to generate a separate interface just for the server and then have that either blocking or suspending controlled via its own flag (in case you want to build a non-blocking thrift server e.g. using ktor), but not generate a callback-based implementation.

That means I need to copy the method that generates the client interface and adjust it for server generation. Also not great, but better than the current state I guess.

This should be fixed now.

benjamin-bader · 2021-05-24T06:21:09Z

thrifty-kotlin-codegen/src/main/kotlin/com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt

+
+        spec.addCode {
+            beginControlFlow("else -> ")
+            addStatement("""throw %T("%L")""", IllegalArgumentException::class, "Unknown method \${msg.name}")


Be careful with using Foo::class as type arguments. Kotlinpoet has an iffy track record with understanding, for example kotlin.Exception - historically it will often end up emitting references to the underlying JVM type, which is ok I guess for this use-case but forecloses on MPP servers too.

tl;dr let's add an entry to the ClassNames object up around line 110 of this file and use that instead.

I see a few other places in the PR (and probably in my code, too, let's be honest) where ::class literals are being used this way. So long as they aren't typealiases or expect/actual types, it's probably OK, but we should make sure.

Honestly I have almost no experience with Kotlin Multiplatform, so I tried to do what you said to the places I found :)

thrifty-runtime/src/commonMain/kotlin/com/microsoft/thrifty/ThriftException.kt

luqasn · 2021-06-14T21:18:14Z

Sorry for the long delay, was struggling to find the motivation to sit down after work to get this done :)
Tried to take care of all the feedback, the coroutineServiceClient thingy will need to wait for another evening, though it should not require a lot of work/brain power.

codecov-commenter · 2021-06-14T21:19:17Z

Codecov Report

Merging #447 (dffd769) into master (e9ec1fb) will increase coverage by 1.11%.
The diff coverage is 67.74%.

@@             Coverage Diff              @@
##             master     #447      +/-   ##
============================================
+ Coverage     57.27%   58.38%   +1.11%     
- Complexity      789      841      +52     
============================================
  Files            67       68       +1     
  Lines          5746     6421     +675     
  Branches        854      957     +103     
============================================
+ Hits           3291     3749     +458     
- Misses         2233     2397     +164     
- Partials        222      275      +53

Impacted Files	Coverage Δ
.../com/microsoft/thrifty/compiler/ThriftyCompiler.kt	`0.00% <0.00%> (ø)`
...hrifty/gradle/GenerateThriftSourcesWorkAction.java	`0.00% <0.00%> (ø)`
.../microsoft/thrifty/gradle/KotlinThriftOptions.java	`0.00% <0.00%> (ø)`
...soft/thrifty/gradle/SerializableThriftOptions.java	`0.00% <0.00%> (ø)`
...java/com/microsoft/thrifty/testing/TestClient.java	`60.46% <60.46%> (ø)`
.../com/microsoft/thrifty/kgen/KotlinCodeGenerator.kt	`84.00% <97.82%> (+1.41%)`	⬆️
...tlin/com/microsoft/thrifty/schema/ServiceMethod.kt	`67.60% <100.00%> (+11.00%)`	⬆️
.../kotlin/com/microsoft/thrifty/schema/StructType.kt	`44.00% <0.00%> (ø)`
.../com/microsoft/thrifty/schema/FieldNamingPolicy.kt	`90.47% <0.00%> (+2.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9ec1fb...dffd769. Read the comment docs.

luqasn · 2021-06-18T11:13:37Z

Server generation now always generates its own service interface in a server namespace that is always coroutine-based.
First, I tried for allowing both "plain" service handlers and coroutine ones, but that was a pain in the back because I had to duplicate a lot of code because suspend functions have different signatures.
So in the end I opted to just always generate coroutine based servers, because it allows for more freedom for the user, e.g. to use reactive constructs, either magic from Arrow or whatever.
If you choose to not use any coroutine specific stuff in your handler, that should be totally fine and not impact performance much, all you have is an extraneous parameter on your methods for the coroutine continuation (or maybe the compiler even optimizes that away if you never suspend).

luqasn · 2021-07-07T20:50:24Z

I have addressed your feedback (thanks a lot!) and added the missing license headers.

benjamin-bader · 2021-07-09T19:53:35Z

Nice, thank you! This generally looks good. I'm going to hold off on merging this, however, until after we finalize a 3.0.0 release. That should be fairly soon, but I'm still working out some problems with it.

benjamin-bader · 2021-08-10T03:27:36Z

A whole month later, I've finally released 3.0.0. Thanks for your extraordinary patience. Let's get this landed!

and make kind required (there is an "unknown" type anyways)

…te to them

in case the generator gets re-used

in order to try to be as compatible with Kotlin Multiplatform as possible.

in a server-specific namespace instead of reusing the client interface. This is necessary because we always want to generate a coroutine based interface for the server and never a callback based one.

because now we generate a separate FileSpec for the server that also need the types from the "normal" FileSpec.

as it no longer requires the coroutine client flag to be set.

luqasn · 2021-08-12T19:09:25Z

I have pulled your changes in via a rebase fixing any conflicts and only after I was done did I realize that that's not the most transparent way for you to track what has changed. Was totally in my work-workflow, sorry. I guess you would have probably preferred a merge commit?

benjamin-bader · 2021-08-12T21:05:47Z

README.md

+`--experimental-kt-generate-server` enabled code generation for the server portion of a thrift service. You can
+use this to implement a thrift server with the same benefits as the kotlin client: no runtime surprises thanks to
+structs being always valid by having nullability guarantees and unions represented as sealed classes.
+See 
+
+### Thanks


This readme block looks misplaced. Is this an artifact of the rebase?

yes, seems I somehow messed that up. Fixed now.

luqasn · 2021-09-15T07:24:37Z

Bump :)

benjamin-bader · 2021-09-15T14:05:17Z

oh geez, were you waiting for me? I thought I was waiting for you 😧
let me take a look again

benjamin-bader

benjamin-bader reviewed May 24, 2021

View reviewed changes

luqasn added 16 commits August 12, 2021 20:15

Expose synthetic structs for method args and result

6651238

Make ThriftException a Struct

d27539c

and make kind required (there is an "unknown" type anyways)

Move type holders to class level in order to allow for methods to wri…

9d8219d

…te to them

Add runtime classes for server support

3eadfa6

Add server support

ac191ac

Update README

6270ac4

Clear type outputs at the beginning of generate()

164e4b0

in case the generator gets re-used

clean up imports

929ba32

remove @jvmoverloads

a4a8912

import all the things

ce1d3f4

Remove wildcard import

06bfb66

Use hand-built ClassNames instead of direct type references

f1d69e2

in order to try to be as compatible with Kotlin Multiplatform as possible.

Generate server-specific interface

8b05dc7

in a server-specific namespace instead of reusing the client interface. This is necessary because we always want to generate a coroutine based interface for the server and never a callback based one.

Support compilation of multiple FileSpecs as a unit

b1cfe1e

because now we generate a separate FileSpec for the server that also need the types from the "normal" FileSpec.

Move server integration test to "normal" task

fa3aad1

as it no longer requires the coroutine client flag to be set.

Add license headers

6fadf97

luqasn force-pushed the server-support branch from c4b3281 to 6fadf97 Compare August 12, 2021 18:54

Add license to thrift test code

c21f74d

benjamin-bader reviewed Aug 12, 2021

View reviewed changes

Fix README

dffd769

benjamin-bader approved these changes Sep 18, 2021

View reviewed changes

benjamin-bader merged commit b28da74 into microsoft:master Sep 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Server support #447

Add Server support #447

luqasn commented Apr 10, 2021 •

edited

Loading

codecov-io commented Apr 10, 2021

luqasn commented Apr 10, 2021 •

edited

Loading

luqasn commented Apr 11, 2021 •

edited

Loading

luqasn commented May 21, 2021

benjamin-bader left a comment

benjamin-bader Apr 12, 2021

benjamin-bader May 24, 2021

luqasn Jun 14, 2021

benjamin-bader May 24, 2021

luqasn Jun 14, 2021

benjamin-bader May 24, 2021

luqasn Jun 14, 2021

benjamin-bader May 24, 2021

luqasn Jun 14, 2021

luqasn Jun 18, 2021

benjamin-bader May 24, 2021

benjamin-bader May 24, 2021

luqasn Jun 14, 2021

luqasn commented Jun 14, 2021 •

edited

Loading

codecov-commenter commented Jun 14, 2021 •

edited

Loading

luqasn commented Jun 18, 2021 •

edited

Loading

luqasn commented Jul 7, 2021

benjamin-bader commented Jul 9, 2021

benjamin-bader commented Aug 10, 2021

luqasn commented Aug 12, 2021

benjamin-bader Aug 12, 2021

luqasn Aug 15, 2021

luqasn commented Sep 15, 2021

benjamin-bader commented Sep 15, 2021

benjamin-bader left a comment


		fun generate(schema: Schema): List<FileSpec> {
		val specsByNamespace = LinkedHashMultimap.create<String, TypeSpec>()

		import okio.Buffer


		class TestServer @JvmOverloads constructor(private val protocol: ServerProtocol = ServerProtocol.BINARY) : Extension, BeforeEachCallback, AfterEachCallback {

Add Server support #447

Add Server support #447

Conversation

luqasn commented Apr 10, 2021 • edited Loading

codecov-io commented Apr 10, 2021

Codecov Report

luqasn commented Apr 10, 2021 • edited Loading

luqasn commented Apr 11, 2021 • edited Loading

luqasn commented May 21, 2021

benjamin-bader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luqasn commented Jun 14, 2021 • edited Loading

codecov-commenter commented Jun 14, 2021 • edited Loading

Codecov Report

luqasn commented Jun 18, 2021 • edited Loading

luqasn commented Jul 7, 2021

benjamin-bader commented Jul 9, 2021

benjamin-bader commented Aug 10, 2021

luqasn commented Aug 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luqasn commented Sep 15, 2021

benjamin-bader commented Sep 15, 2021

benjamin-bader left a comment

Choose a reason for hiding this comment

luqasn commented Apr 10, 2021 •

edited

Loading

luqasn commented Apr 10, 2021 •

edited

Loading

luqasn commented Apr 11, 2021 •

edited

Loading

luqasn commented Jun 14, 2021 •

edited

Loading

codecov-commenter commented Jun 14, 2021 •

edited

Loading

luqasn commented Jun 18, 2021 •

edited

Loading