Skip to content

More proper overloads with suspend handlers #413

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

Open
nerzhulart wants to merge 1 commit into
base: master
Choose a base branch
from
Open

More proper overloads with suspend handlers #413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 26 additions & 0 deletions .../rd-framework/src/main/kotlin/com/jetbrains/rd/framework/util/IRdEndpointCoroutineUtil.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,13 @@ import com.jetbrains.rd.framework.impl.RdEndpoint
import com.jetbrains.rd.util.lifetime.Lifetime
import com.jetbrains.rd.util.reactive.IScheduler
import com.jetbrains.rd.util.threading.SynchronousScheduler
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.CoroutineStart
import kotlinx.coroutines.async
import kotlin.coroutines.CoroutineContext
import kotlin.coroutines.EmptyCoroutineContext

@Deprecated("Use the overload with CoroutineScope and CoroutineContext and pass all required context elements")
Copy link
Collaborator

@Iliya-usov Iliya-usov Jun 15, 2023

Choose a reason for hiding this comment

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

I don't see any reason do deprecate this method, and more over force people to pass CoroutineScope here

fun <TReq, TRes> IRdEndpoint<TReq, TRes>.setSuspend(
cancellationScheduler: IScheduler? = null,
handlerScheduler: IScheduler? = null,
Expand All @@ -21,4 +26,25 @@ fun <TReq, TRes> IRdEndpoint<TReq, TRes>.setSuspend(
set(cancellationScheduler, handlerScheduler) { lt, req ->
lt.startAsync(coroutineDispatcher) { handler(lt, req) }.toRdTask()
}
}

/**
* Sets suspend handler for the [IRdEndpoint].
*
* When a protocol call is occurred it starts a new coroutine on [scope] passing [coroutineContext] and [coroutineStart] to it.
* [cancellationScheduler] and [handlerScheduler] are passed to [IRdEndpoint.set]
*/
fun <TReq, TRes> IRdEndpoint<TReq, TRes>.setSuspend(
Copy link
Collaborator

@Iliya-usov Iliya-usov Jun 15, 2023

Choose a reason for hiding this comment

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

To be honest, I find this method a little strange

  1. CoroutineContext often encompasses a dispatcher. passing coroutineContext + handlerScheduler looks contradictory.

  2. I don't see any reason to pass outer scope here. This rd-call handler has his own scope/lifetime, which is cancelled when the counterpart scope/lifetime is cancelled, having two sopes looks also looks contradictory to me.

  3. In your implementation we lose coroutine cancellation, because you don't use a lifetime received with the request to create the coroutine.

  4. In this method we first go to the handlerScheduler (the protocol scheduler in most cases) and only after that we go to coroutineContext, in some cases this can be a problem if for example the main thread is stuck, but we don't really need the main thread at all, this can cause performance problems.

So I believe that we don't need that kind of API in the platform, because that API has a lot of non-obvious and contradictory moments, but at the same time it's easy to write that kind of code in your specific case.

I would prefer that kind of API:

fun <TReq, TRes> IRdEndpoint<TReq, TRes>.setSuspend(coroutineContext: CoroutineContext, handler: suspend CoroutineScope.(Lifetime, TReq) -> TRes) {
    val dispatcher = coroutineContext[ContinuationInterceptor] as? CoroutineDispatcher
    requireNotNull(dispatcher) { "coroutineContext: $coroutineContext doesn't have a CoroutineDispatcher" }
    
    val scheduler = dispatcher.asRdScheduler
    set(cancellationScheduler = SynchronousScheduler, scheduler) { lt, req ->
        lt.startAsync(coroutineContext, CoroutineStart.UNDISPATCHED) { handler(lt, req) }.toRdTask()
    }
}

Or we can use protocol dispatcher in case if coroutine context doesn't have corotineDispatcher

scope: CoroutineScope,
coroutineContext: CoroutineContext = EmptyCoroutineContext,
coroutineStart: CoroutineStart = CoroutineStart.DEFAULT,
cancellationScheduler: IScheduler? = null,
handlerScheduler: IScheduler? = null,
handler: suspend (Lifetime, TReq) -> TRes
) {
set(cancellationScheduler, handlerScheduler) { lt, req ->
scope.async(coroutineContext, coroutineStart) {
handler(lt, req)
}.toRdTask()
}
}
11 changes: 7 additions & 4 deletions rd-kt/rd-framework/src/main/kotlin/com/jetbrains/rd/framework/util/ISourceCoroutineUtil.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import com.jetbrains.rd.util.lifetime.Lifetime
import com.jetbrains.rd.util.reactive.IScheduler
import com.jetbrains.rd.util.reactive.ISource
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.CoroutineStart
import kotlinx.coroutines.Deferred
import kotlinx.coroutines.launch
import kotlin.coroutines.CoroutineContext
import kotlin.coroutines.EmptyCoroutineContext

fun <T> ISource<T>.nextValueAsync(
lifetime: Lifetime,
Expand Down Expand Up @@ -53,13 +54,15 @@ suspend fun <T : Any> ISource<T?>.nextNotNullValue(lifetime: Lifetime = Lifetime
nextNotNullValueAsync(lifetime).await()
}

@Deprecated("Use overload with CoroutineContext")
fun<T> ISource<T>.adviseSuspend(lifetime: Lifetime, scheduler: IScheduler, handler: suspend (T) -> Unit) {
adviseSuspend(lifetime, scheduler.asCoroutineDispatcher(allowInlining = true), handler)
// AIR allowInlining is a bad practice, ask @daniil for the details
adviseSuspend(lifetime, scheduler.asCoroutineDispatcher(allowInlining = true), CoroutineStart.UNDISPATCHED, handler)
}

fun<T> ISource<T>.adviseSuspend(lifetime: Lifetime, context: CoroutineContext, handler: suspend (T) -> Unit) {
fun<T> ISource<T>.adviseSuspend(lifetime: Lifetime, context: CoroutineContext = EmptyCoroutineContext, coroutineStart: CoroutineStart = CoroutineStart.DEFAULT, handler: suspend (T) -> Unit) {
advise(lifetime) {
lifetime.launch(context) {
lifetime.launch(context, coroutineStart) {
handler(it)
}
}
Expand Down