[ETCM-269] future task

[lemastero]

Description

Replace Future with Task in JSON RPC.

Proposed Solution

Important Changes Introduced

Testing

[rtkaczyk]

When done, could you please provide a brief argument for the choice of Monix rather than cats-effect or ZIO?

[kapke]

One more thing, that might be useful - in service specs you might use SpecBase traits and for sync, blocking specs write a wrapper like done there: https://github.com/input-output-hk/mantis/pull/735/files#diff-4f204c964a1f172070b2e06bc6b410e888aa541c50fe67c44824d5028d1e82d8R59. So new specs are free to return Task and not bother about actually executing it

[lemastero]

could you please provide a brief argument for the choice of Monix rather than cats-effect or ZIO?

I am strongly convinced ZIO is better.

• very detailed thread dumps
• explicit errors (you can use Task[Either[E,A]] but you need nested maps/flatmaps to handle error, ZIO expose directly methods to deal with that, and in sense, you are encouraged to say what error type is)
• capabilities of FP patterns like monad transformers, Kleisli, Contravariant, Functor, Profunctor, Arrow, Monad, MonadError, Compose are included in ZIO and hence is easier and more performant
• you can change input/environment and that is not possible neither with Monix/cats-effect nor with Monix BIO
• ZIO has consistent design, always input, output and error channel not only in ZIO but in other abstractions

monix.Task because:

• it is already in the project
• it requires the least changes (when changing Future to Task you need to just change name + take care of differences in laziness and slightly different API) with ZIO there is more changes (you can model better error, because you can model input, functions become val, ...)
• fewer changes == better review
• fewer changes == less opportunity to make error

Why monix.Task not cas-effect - I think abstracting over effect type sounds good in theory but:

• is way more complex in terms of wiring, has a more steep learning curve. I was on project with monix, I was on project with monix + CE and the latter was significantly more complex. I like fancy types in libraries/frameworks - more powerful, more general. I prefer simple code in production = easier to maintain.
• IMHO using CE to be able to switch IO monad implementation from Task to ZIO sounds good but is not great. ZIO powers come from modeling explicitly errors and input + consistent design + use data types instead of traits (speed). CE has the same design as monix. So the only reasonable IO type implementation for CE is Task.
• switching from Task to ZIO is simpler (in theory you can import zio.Task instead of monix.eval.Task in the simplest approach) compared to CE to ZIO

I am willing to try to port code to ZIO and see by myself what would be practical benefits for mantis and come back to this discussion. Yet for now, Task seems like a good choice (stepping stone) :)

[kapke]

Looks good to me!
While I do think tests could be improved a bit, I think that they can be improved separately from this PR, even if organically when working on RPC

[mmrozek]

Do we want to use strict evaluation here? Shouldn't be Task.deferFuture?

[lemastero]

Very good question 🤔

[lemastero]

This breaks the tests, I will explore what is needed to fix that.

[lemastero]

Changed to Task.deferFuture: 7494676

[mmrozek]

Why runToFuture? To be consistent we should use runSyncUnsafe everywhere (and avoid futureValue)

[lemastero]

runSyncUnsafe was implemented at the beginning. But this break the tests:

[info] DebugServiceSpec:
[info] DebugService
[info] - should return list of peers info *** FAILED *** (8 seconds, 376 milliseconds)
[info]   java.lang.AssertionError: assertion failed: timeout (3 seconds) during expectMsg while waiting for PeerInfoRequest(PeerId(testProbe-4))
[info]   at scala.Predef$.assert(Predef.scala:223)
[info]   at akka.testkit.TestKitBase.expectMsg_internal(TestKit.scala:459)
[info]   at akka.testkit.TestKitBase.expectMsg(TestKit.scala:436)
[info]   at akka.testkit.TestKitBase.expectMsg$(TestKit.scala:436)
[info]   at akka.testkit.TestKit.expectMsg(TestKit.scala:969)
[info]   at io.iohk.ethereum.jsonrpc.DebugServiceSpec$$anon$1.<init>(DebugServiceSpec.scala:35)
[info]   at io.iohk.ethereum.jsonrpc.DebugServiceSpec.$anonfun$new$1(DebugServiceSpec.scala:28)
[info]   at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
[info]   at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)
[info]   at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
[info]   ...

Some tests contain a mix of:

• expectations on Akka TestProbe
• sending messages to actors
• assertions at the end.

In some cases, I moved expectations and assertions at the end. But in some cases, I was not able to do it so I used conversion to Future to get async behavior.

[mmrozek]

LGTM!