CoroutineContext.effect doesn't switch back to caller thread

[aballano]

Maybe this is a self-assumption but I think it would be really helpful to avoid multiple thread switches, specially on "UI-driven" frameworks such as Android.

Quick example, having:

private suspend fun printCurrentThread() {
    println(Thread.currentThread().name)
}

val result1 = fx {
    !effect { printCurrentThread() }
    !NonBlocking.effect { printCurrentThread() }
    !effect { printCurrentThread() }
}

fun main() {
    unsafe { runBlocking { result1 } }
}

At least from a newbie point of view I'd expect the output to be (when called from the main thread):

main
ForkJoinPool-1-worker-1
main

But actually is:

main
ForkJoinPool-1-worker-1
ForkJoinPool-1-worker-1

This is due to the effect ext fun behaving similarly to continueOn, although I'd expect that ideally only the lambda inside is executed in the NonBlocking context and then returning to the caller context (main in this case).

[pakoito]

That's why I preferred the continueOn approach, it's harder to get lost on assumptions. I'd say remove that override.

[aballano]

@pakoito The thing is that this approach would be pretty handy for mobile or UI apps since it saves you from doing (or forgetting)

val result1 = fx {
    !effect { setViewLoading() }
    continueOn(NonBlocking)
    val data = !effect { retrieveSomeData }
    continueOn(Main)
    !effect { showDataInView(data) }
}

It is actually one of the reasons why I had to create this 2 dumb extensions in my experiment, otherwise I might forget switching a thread at some point and get some unexpected exceptions.

Although I'm not sure how complex would be to have that behaviour described in the issue.

[pakoito]

Forgetting to go back to the Main thread isn't a problem we need to solve in the library I believe. We can add that extfun that does both jumps, sure, and still keep what we have today. When we have the arrow-ui-android package we could make a nicer DSL with these assumptions.

The more I think about adding a mandatory continueOn to NonBlocking.effect, the less confident I feel that's a good idea.

[JorgeCastilloPrz]

We definitely don't want to "pollute" the lib API as is just because of Android @pakoito. Actually on my mind this was more about adding it as sugar in the arrow-android artifact, since 90% of bg ops we do in Android require getting back to Main thread afterwards (not exactly main, but the caller thread, so it also applies to tests).

[aballano]

Sorry I didn't read this before submitting arrow-kt/arrow#1395 😅

I agree, if this is not a need for non-Android apps (not sure about desktop JavaFx and so) then I agree it doesn't make sense to pollute the arrow-effects core with it.

For me it's perfectly fine to stall the PR and wait for the android module 👍. It could also allow us, as @pakoito suggested, to use this assumptions to, for example, default to the Main thread after an effect 👌

[JorgeCastilloPrz]

You're PR could also be a good excuse to add an early arrow-android module @aballano :) (in case people agrees on the proposed approach)

[nomisRev]

That's why I preferred the continueOn approach, it's harder to get lost on assumptions. I'd say remove that override.

+1, I've seen some really big messes with implicit threading. It can turn in a very big price tag, $ and CPU wise.

I personally prefer this to be as explicit as possible. It's not typed, so I need to dive into the functions to see what they do etc.
Some other downsides I see now but may potentially be fixed.

We optimize ContinueOn to skip multiple sequential calls, to only run the most last one.

IO.ContinueOn(DatabaseCtx)
  .flatMap { IO.ContinueOn(NetworkCtx) } 
  .followedBy(IO.ContinueOn(Main)) == IO.ContinueOn(Main)

This might degrade depending on the solutions used. The more complex the solution is, adding a case to the IO ADT and implement it in the run loop and figure out a sensible impl. (On the top of my head).

Another case against or an alternative solution which I'd prefer. Is was something @raulraja came up with a while ago. I am not sure what the plans are with it tho.

[raulraja]

arrow-kt/arrow#1395 (comment)

[pakoito]

#1395 (comment)

There's no such thing with coroutines. If the current context is EmptyCoroutineContext, the default, using it won't make it come back to the same thread. It requires installing event loops on arbitrary threads to make it work end-to-end, and IIRC we don't have those, although it rings a bell that we did something similar recently. I believe RxJava removed theirs for some reason.

[JorgeCastilloPrz]

I think we should focus the discussion in one place, if possible.

[JorgeCastilloPrz]

So, is there a decision on this? so we can try to move forward.

[pakoito]

Simon changed to event loops in Fx/EnvIO, but that's implementation-specific. We'll need to revisit it properly.

[nomisRev]

I had a small discussion with @JorgeCastilloPrz on Slack the other day. We'd propose this API, and thus remove CoroutineContext.effect { } and keep a smaller API surface to do actual threading. Beside the below described combinators the only other onces that do threading can be in Concurrent and are fork, racePair, raceTriple, parMapN, raceN.

// Runs receiver Kind<F, A> on ctx and returns to original thread
fun <F, A> Kind<F, A>.evalOn(ctx: CoroutineContext): Kind<F, A>

// Continue execution on ctx *after* receiver Kind<F, A>.
fun <F, A> Kind<F, A>.continueOn(ctx: CoroutineContext): Kind<F, A>

// Stand alone context switch modeled as IO. Alias for `IO.unit.continueOn(ctx)`
fun <F> CoroutineContext.shift(): Kind<F, Unit>

// Could be used for modifying the `CoroutineContext` key-value pairs but can be used for threading as well. i.e. `program.continueOn(NonBlocking) == program.updateContext { _ -> NonBlocking }`
fun <F, A> Kind<F, A>.updateContext(f: (CoroutineContext) -> CoroutineContext): Kind<F, A>

Separate discussion: should racing and parMap return to the original context?

[nomisRev]

@pakoito

I believe RxJava removed theirs for some reason

RxJava doesn't do any event looping and thus also never returns to an original thread so brining evalOn in the typeclass hierarchy might proof to be a fun one..

[JorgeCastilloPrz]

Looks good to me. I'm always up for simpler API surfaces instead of having N solutions for "the same thing". evalOn would help specially on platforms like Android where we're used to run something in bg then always come back to main thread.

@nomisRev would there be any way of ensuring evalOn can just be called over effect { } ? right now I guess effect { } returns kind but it might be worth to ensure you can just eval on a different thread properly wrapped effects. Would that make sense?

[pakoito]

You'll need to make evalOn suspend if you want to retrieve the current context, or implement it only for fx and capture it from the Continuation.

The race and parallel returning to "current context" have the same problem, you'll need to grow the API with several context injections that default to trampoline for normal use cases, when what you actually want is to continueOn the result of the parallelization/race sometimes.

[nomisRev]

Implementing evalOn is tricky and for this reason also parked until after we merge the PR at least.
See Paco's comment above.

@JorgeCastilloPrz it doesn't really make a lot of sense to me since a lot of people will still just work with Kind directly. Like when working with the concurrency primitives, or other tagless constructs/programs.

Also I am not sure I understand what you mean with

you can just eval on a different thread properly wrapped effects

As in do a runtime check to avoid redundant jumps? That would be really confusing because some people might want to do excessive switching because of fairness. We do this optimization inContinueOn but that is before actually running the code.

[JorgeCastilloPrz]

@JorgeCastilloPrz it doesn't really make a lot of sense to me since a lot of people will still just work with Kind directly. Like when working with the concurrency primitives, or other tagless constructs/programs.

👍 fair point

As in do a runtime check to avoid redundant jumps? That would be really confusing because some people might want to do excessive switching because of fairness. We do this optimization inContinueOn but that is before actually running the code.

Nono I didn't wanna to go that deep, it was simpler than that tbh. It was more in the line of having a way to ensure people can't make thread jumps work in fx in any way for suspended functions that are not wrapped in effect { }, but that's kind of implicit with the current encoding, given effect { } lifts it to Kind and that's the receiver of evalOn.

The only limitation there would be that Kind does not really ensure it's a safely wrapped effect (it could be an eager one). Looks like that woulnd't be possible as in evalOn would be defined for Async scopes so just data types that support async would stand for that Kind. So as long as we don't have any async supporting data types that are eager (something like an eager future) we'd already be achieving the goal.

[nomisRev]

This requires an EventLoop similar to KotlinX's EventLoop in runBlocking

[nomisRev]

Closed due to obsolete