rsocket / rsocket-kotlin

RSocket Kotlin multi-platform implementation
http://rsocket.io
Apache License 2.0
552 stars 37 forks source link

RSocketRequestHandlerBuilder is inconsistent with suspending functions #143

Closed yschimke closed 3 years ago

yschimke commented 3 years ago

https://github.com/rsocket/rsocket-kotlin/blob/d212568589633064809dc928976cdcbbf222b97b/rsocket-core/src/commonMain/kotlin/io/rsocket/kotlin/RSocketRequestHandler.kt#L27-L28

The Flow forms of callbacks are not suspending, while this is accurate as Flow allows deferring any suspending work, it's awkward in a library. If a suspending block is used instead and the defer is done inside the framework code it makes it a lot simpler for clients.

Functions of the form below are common in practice.

suspend fun x(): Flow<String>
whyoleg commented 3 years ago

I agree, that it will be good to allow using some suspend fun f(): Flow<*> for request handler. And it's a good addition for request handler builder. The main reason, why RSocket interface don't have suspend is that from client(requester) side, until flow isn't collected, it will not do anything, and suspending is not needed. Will you create PR for this for RSocketRequestHandler only?

yschimke commented 3 years ago

I agree it's technically superfluous but it makes it much simpler. I've worked around with

                    flow {
                      emitAll(runCommand(request).map {
                        buildPayload {
                          data(responseAdapter.toJson(it))
                        }
                      })
                    }

It's a bit like Mono.defer(...) in reactor

yschimke commented 3 years ago

I also looked at having overloaded forms of the methods (suspend + normal) on the builder, but it seems that it frequently confuses Intellij at call sites.

whyoleg commented 3 years ago

fixed in #144