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

Fix Task/Coeval memoize operation #213

Merged
merged 3 commits into from Aug 23, 2016

Conversation

Projects
None yet
1 participant
@alexandru
Copy link
Member

commented Aug 23, 2016

From a conversation from August 22, 2016 8:08 PM with @mtomko:

    import monix.eval.Task
    import monix.execution.Scheduler.Implicits.global

    import scala.concurrent.Await
    import scala.concurrent.duration._

    def fprint[A](a: => A) {
      println(a.toString)
      System.out.flush()
    }

    // Memoization test:
    val task = Task { fprint("Hello"); 3 }.memoize
    val task2 = task map { x => fprint("Goodbye"); x + 1 }

    println(Await.result(task2.runAsync, Duration.Inf))
    println("========")
    println(Await.result(task2.runAsync, Duration.Inf))

Yields this output:

Hello
Goodbye
4
========
Goodbye
5

The problem is in the trampoline implementation, introduced in the optimizations done for RC11. Hopefully we'll end up having a code coverage so good that such problems will no longer be possible.

Also Coeval.memoize was lacking a stack-overflow test and consequently we didn't notice that the implementation is not stack-safe. Fix is easy.

As part of the fix, I deleted both EvalAlways and EvalOnce states in Task. This is because in Task these states have no benefit and can be described just with Suspend, while deleting these states basically reduces the area that we have to cover with tests. On the other hand for Coeval, we still need them in order to describe memoization effectively, because in Coeval we cannot just asynchronous boundaries whenever we feel like it.

alexandru added some commits Aug 23, 2016

@alexandru alexandru added this to the 2.0 milestone Aug 23, 2016

@alexandru alexandru merged commit 48ffbab into master Aug 23, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@alexandru alexandru deleted the fix-memoize branch Nov 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.