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 a purity breakage in unconsAsync, resulting in wild behaviour #1037

Merged
merged 3 commits into from
Dec 28, 2017

Conversation

SystemFw
Copy link
Collaborator

Anything involving unconsAsync (e.g. merge, interruptWith) was affected, and the side-effecting behaviour resulted in weird things happening. As an example:

object Bug {
  import fs2._
  import cats._, implicits._
  import cats.effect._

  import scala.concurrent.ExecutionContext.Implicits.global
  import scala.concurrent.duration._

  def stepAsync(s: Stream[IO, Unit]): Stream[IO, Unit] =
    s.pull.unconsAsync.flatMap { ap =>
      ap.pull.flatMap {
        case None => Pull.done
        case Some((hd, tl)) => Pull.output(hd) >> tl.pull.echo
      }
    }.stream

  def stream = Scheduler[IO](2).flatMap { s =>
    val a = s.sleep_[IO](1.seconds) ++ Stream.eval(IO(println("tick")))

    //side effects
    val broken = stepAsync(a)
    def works = stepAsync(a)

    broken ++ broken ++ broken ++ works ++ works ++ works ++ broken ++ broken ++ broken
  }.run.unsafeRunSync
}

all the broken except the first would complete immediately.

I would have liked to add a test for this, but struggling for ideas to make it nice

type Res = Option[(Segment[O,Unit], Stream[F,O])]

Pull.eval(Promise.empty[F, Either[Throwable, Res]]) flatMap { p =>
Pull.fromFreeC {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could inline this Pull:

Pull.fromFreeC { Algebra.eval[F, Nothing, Promise[F, Res]](async.Promise.empty).flatMap { p =>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah that's a good idea, I'll do that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact, the whole operation seems quite similar to start, so maybe it's worth thinking for a while how to make it a bit nicer

@avakhrenev
Copy link
Contributor

About tests: there only 2 tests for Scheduler. Maybe you could add a couple of tests for other Scheduler methods, which also demonstrate this issue?

@SystemFw
Copy link
Collaborator Author

SystemFw commented Dec 28, 2017

Note that e.g. retry has its own set of tests, and now that I look at it, there's no reason SchedulerSpec should be in jvm and not in shared, and be perhaps unified with RetrySpec (more items for #1009 )

It's probably a good idea to have more tests for that, but to test the behaviour exhibited here I'd have to involve both a scheduler and merge, plus something like repeat... it feels a bit weird testing for referential transparency :)

@pchlupacek
Copy link
Contributor

lgtm. I don't think so we need tests for that a such it was more like escaping the ref transparency bounds.

@SystemFw
Copy link
Collaborator Author

let me reformat real quick

@pchlupacek pchlupacek merged commit 11b36e0 into typelevel:series/0.10 Dec 28, 2017
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.

3 participants