reworks implementation using coroutines and multiplatform #76

whyoleg · 2020-07-26T11:22:04Z

Minimal multi-platform implementation based on coroutines and Ktor
Works on:

server
- JVM: TCP/WS
client
- JVM: TCP/WS
- JS: WS

OlegDokuka · 2020-07-26T13:55:12Z

@sdeleuze will appreciate your review! 🙇‍♂️

yschimke · 2020-07-26T14:13:00Z

This is massive, love this.

CommanderTvis · 2020-07-29T09:40:22Z

That's a big step forward.

sdeleuze · 2020-07-29T11:03:04Z

OMG, is it real?

I will review it asap (possible delays due to my upcoming vacations).

OlegDokuka

@whyoleg can you please change all the packages to have the base io.rsocket?

Along, can we add the same license headers to all files as we have it at rsocket-java?

whyoleg · 2020-07-29T11:20:35Z

Yeah, will do on a weekend (on vacation now)

yschimke · 2020-08-01T14:37:10Z

rsocket-core/src/commonMain/kotlin/io/rsocket/Cancelable.kt

+
+val Cancelable.isActive: Boolean get() = job.isActive
+
+fun Cancelable.cancel(cause: CancellationException? = null) {


curious: Why so many extension methods, instead of defaults on the interface.

Mainly code style. But also, default methods can be overridden, so it's even safer to keep them as extensions.
Also, it's more kotlinish

I wouldn't say it's kotlin idiomatic, which is why I asked. But not an issue, just curious.

yschimke · 2020-08-01T14:38:32Z

rsocket-core/src/commonMain/kotlin/io/rsocket/client/RSocketClientConfiguration.kt

+    val ignoredFrameConsumer: (Frame) -> Unit = {}
+)
+
+fun RSocketClientConfiguration.setupFrame(): SetupFrame = SetupFrame(


curious: again, why an extension method?

similar to upper comment, but here, it's even can be just inlined in implementation.

yschimke · 2020-08-01T15:03:40Z

This is a tough big review with a lot of formatting, copyright changes. I won't be doing a deep dive, but once we have snapshots for this I'd love to start testing it against various servers (rsocket-demo, cli) and platforms (JS, Java).

OlegDokuka · 2020-08-01T16:42:00Z

@yschimke @sdeleuze @rstoyanchev I have done the first round of review with @whyoleg - so far so good. I can propose to schedule a call with everyone interested in the review to make grasping of the ideas easier. Let me know your thoughts on that

yschimke · 2020-08-01T16:44:10Z

I'll probably wait until it lands and kick the tires then. This is really great to see!

OlegDokuka · 2020-08-01T16:48:28Z

Also, I'm not a Kotlin expert, so my review is more structure/features/ideas related rather than the idiomatic and best practice of the Kotlin usage oriented

Thus, it is only you @yschimke and @sdeleuze who may bring better Kotlin usage into the place

merge client/server to core package minor cleanup payload builder

yschimke · 2020-08-04T08:06:53Z

I'd vote for landing and iterating. I'm keen to test this with my rsocket-cli and flush out issues against rsocket-java server etc.

frames tests fixing dependencies add using of experimental annotations in tests

yschimke · 2020-08-06T06:11:25Z

What is the background with the build system, is this standard for kotlin MP projects?

edit: found https://github.com/whyoleg/kamp

yschimke · 2020-08-06T06:28:55Z

Nice, just works against the demo site

@OptIn(KtorExperimentalAPI::class)
val engine: HttpClientEngineFactory<*> = CIO

suspend fun main() {
  val client = HttpClient(engine) {
    install(WebSockets)
    install(RSocketClientSupport)
  }

  val rSocket = client.rSocket("wss://rsocket-demo.herokuapp.com/rsocket")
  val response = rSocket.requestStream(Payload.Empty)

  val flow = response.onEach {
    println(it.data.readText())
  }

  flow.collect()
}

1596695278886
1596695278925
1596695278974
1596695279024
1596695279074

yschimke · 2020-08-06T06:30:36Z

Since Kotlin 1.4 is imminent, and presumably ktor bumps. What are thoughts there? Should I wait before trying to use in my code?

whyoleg · 2020-08-06T09:05:01Z

Since Kotlin 1.4 is imminent, and presumably ktor bumps. What are thoughts there? Should I wait before trying to use in my code?

Should be okay, I don't expect any breaking changes in ktor in what used here.
BTW, I will try to publish some test version today.

whyoleg · 2020-08-07T18:48:15Z

@yschimke First published version for testing can be found on bintray.
Feel free to experiment.

yschimke · 2020-08-07T19:25:57Z

Trying it now, thanks.

Don't take this personally, but a custom DSL for builds inside of gradle kotlin is disorienting. If it's to manage the MP builds and ktor jars, understood.

whyoleg · 2020-08-07T19:37:04Z

custom DSL - yeah, I know, it can be not so intuitive. It's mostly needed for managing MPP dependencies:

Declaring it in one line, instead of line per platform (seems will be not needed after 1.4)
To easy find dependencies, what I need. In MPP project, the default stack is coroutines, serialization, ktor. So plugin contains shortcuts for all of them.
- simple version management from one place.

yschimke · 2020-08-07T19:51:14Z

rsocket-core/src/commonMain/kotlin/io/rsocket/RSocket.kt

+ * limitations under the License.
+ */
+
+package io.rsocket


You are going to need a different package, this one's taken :)

We can use or just rsocket (why not?) or as in current repository io.rsocket.kotlin
Not sure about that...

@yschimke do you have any issues with io.rsocket? Are you mixing rsocket-java / kotlin? We discussed that with @whyoleg and I proposed initially to go with io.rsocket unless someone complains. And here this happened. Thus, the question is how much it is critical. If it is critical I would go for kotlin.rsocket or io.rsocket.kotlin

It's a bad idea. Noone does this. How about io.rsocket.kt?

I can't suggest this strongly enough. Don't reuse the package.

do you think kotlin.rsocket will confuse someone?

anyways. I'm good with io.rsocket.kt just wanna have less . in the base pkg name

I'll defer to you both, but just dont overlap.

Let's then use io.rsocket.kotlin, I don't really like kt ending. If it's ok, I will change it soon

Sounds good to me

yschimke · 2020-08-07T20:55:00Z

This is a lovely implementation. Really nice natural API with kotlin flow.

yschimke · 2020-08-07T21:10:23Z

Playing around with a branch here https://github.com/rsocket/rsocket-cli/tree/kotlin

increase timeout for tests add keep alive tests

whyoleg · 2020-08-12T21:50:25Z

New version with changed root package name and group: bintray

Minimal multi-platform implementation based on coroutines and Ktor Works on: server JVM: TCP/WS client JVM: TCP/WS JS: WS

fully reworked using coroutines and multiplatform

eb476d2

OlegDokuka requested review from rstoyanchev, rdegnan and yschimke July 26, 2020 13:53

OlegDokuka added the enhancement label Jul 26, 2020

OlegDokuka added this to the 1.0.0-M1 milestone Jul 26, 2020

OlegDokuka linked an issue Jul 26, 2020 that may be closed by this pull request

Reboot rsocket-kotlin as a multiplatform + Coroutines based project #59

Closed

OlegDokuka self-requested a review July 26, 2020 13:56

OlegDokuka requested changes Jul 29, 2020

View reviewed changes

whyoleg added 2 commits August 1, 2020 16:55

change root package to io.rsocket

9abab2a

add license header

999c3b6

yschimke reviewed Aug 1, 2020

View reviewed changes

replace ByteArrays with ByteReadPacket

d0dbbe9

merge client/server to core package minor cleanup payload builder

fix packets reading

7b5df56

frames tests fixing dependencies add using of experimental annotations in tests

setup publication

f6cfabe

yschimke reviewed Aug 7, 2020

View reviewed changes

whyoleg added 4 commits August 10, 2020 18:23

simple benchmark for java vs kotlin comparison

f1266b1

add GC benchmark

b3cc50f

increase timeout for tests add keep alive tests

gradle 6.6

4ab61b7

change package name: io.rsocket -> io.rsocket.kotlin

3049149

fix license header in benchmarks

0380c45

whyoleg mentioned this pull request Aug 12, 2020

Reboot rsocket-kotlin as a multiplatform + Coroutines based project #59

Closed

OlegDokuka marked this pull request as ready for review August 16, 2020 07:05

OlegDokuka changed the title ~~Fully reworked using coroutines and multiplatform~~ reworks implementation using coroutines and multiplatform Aug 16, 2020

OlegDokuka merged commit 66a027d into rsocket:develop Aug 16, 2020

OlegDokuka pushed a commit that referenced this pull request Aug 20, 2020

reworks implementation using coroutines and multiplatform (#76)

0904b43

Minimal multi-platform implementation based on coroutines and Ktor Works on: server JVM: TCP/WS client JVM: TCP/WS JS: WS


		val Cancelable.isActive: Boolean get() = job.isActive

		fun Cancelable.cancel(cause: CancellationException? = null) {

reworks implementation using coroutines and multiplatform #76

reworks implementation using coroutines and multiplatform #76

Conversation

whyoleg commented Jul 26, 2020

OlegDokuka commented Jul 26, 2020

yschimke commented Jul 26, 2020

CommanderTvis commented Jul 29, 2020

sdeleuze commented Jul 29, 2020 • edited Loading

OlegDokuka left a comment • edited Loading

Choose a reason for hiding this comment

whyoleg commented Jul 29, 2020

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

yschimke commented Aug 1, 2020

OlegDokuka commented Aug 1, 2020 • edited Loading

yschimke commented Aug 1, 2020

OlegDokuka commented Aug 1, 2020 • edited Loading

yschimke commented Aug 4, 2020

yschimke commented Aug 6, 2020 • edited Loading

yschimke commented Aug 6, 2020

yschimke commented Aug 6, 2020 • edited Loading

whyoleg commented Aug 6, 2020

whyoleg commented Aug 7, 2020

yschimke commented Aug 7, 2020 • edited Loading

whyoleg commented Aug 7, 2020

Choose a reason for hiding this comment

whyoleg Aug 8, 2020 • edited Loading

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

yschimke commented Aug 7, 2020 • edited Loading

yschimke commented Aug 7, 2020

whyoleg commented Aug 12, 2020 • edited Loading

sdeleuze commented Jul 29, 2020 •

edited

Loading

OlegDokuka left a comment •

edited

Loading

OlegDokuka commented Aug 1, 2020 •

edited

Loading

OlegDokuka commented Aug 1, 2020 •

edited

Loading

yschimke commented Aug 6, 2020 •

edited

Loading

yschimke commented Aug 6, 2020 •

edited

Loading

yschimke commented Aug 7, 2020 •

edited

Loading

whyoleg Aug 8, 2020 •

edited

Loading

yschimke commented Aug 7, 2020 •

edited

Loading

whyoleg commented Aug 12, 2020 •

edited

Loading