Skip to content
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.

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

More proper overloads with suspend handlers #413

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

Conversation

nerzhulart
Copy link
Contributor

No description provided.

@ForNeVeR ForNeVeR requested a review from Iliya-usov June 13, 2023 20:49
* 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

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


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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants