catbird icon indicating copy to clipboard operation
catbird copied to clipboard

LiftIO Instances for Future and Rerunnable

Open bpholt opened this issue 5 years ago • 5 comments

Would LiftIO instances for Future and Rerunnable be welcomed? I couldn't find laws for LiftIO but these rough implementations seemed to work after some limited testing I did.

implicit val rerunnableLiftIO: LiftIO[Rerunnable] = new LiftIO[Rerunnable] {
  override def liftIO[A](ioa: IO[A]): Rerunnable[A] =
    Rerunnable.fromFuture(LiftIO[Future].liftIO(ioa))
}

implicit val futureLiftIO: LiftIO[Future] = new LiftIO[Future] {
  override def liftIO[A](ioa: IO[A]): Future[A] = {
    val p = Promise[A]

    ioa.unsafeRunAsync {
      case Left(ex) => p.setException(ex)
      case Right(a) => p.setValue(a)
    }

    p
  }
}

bpholt avatar Apr 10 '20 20:04 bpholt

Hi, thanks for the suggestion! If there aren't any laws or other constraints, we should probably piggy back the implementation on the existing functions here:

https://github.com/travisbrown/catbird/blob/master/effect/src/main/scala/io/catbird/util/effect/package.scala

Your implementation will convert the types but stays on the thread pool and we have to clarify what the intended behavior of LiftIO is regarding thread pool shifting. Concretely: Should it just convert the types from IO while staying on the thread pool of the IO or also shift to the thread pool of the type we converting into (e.g. ContextShift[Rerunnable]). I'd argue that the latter is what people expect but I couldn't find a definitive answer in the docs. I'll have to double check how cats-effect and monix behave in this regard.

See my recent bugfix which tries to address exactly this problem: https://github.com/travisbrown/catbird/pull/209

felixbr avatar Apr 12 '20 09:04 felixbr

In my previous message I might have made some wrong assumptions. One of which is that we already have a function IO[A] => Future[A], but we only have the reverse afaik.

I wrote a message to the cats-effect folks on their opinion but in the meantime I've looked into it more.

It should be noted that Rerunnable already has an instance of LiftIO[Rerunnable] as Effect[Rerunnable] implies Async[Rerunnable] and that implies LiftIO[Rerunnable]. So something like IO.pure("foo").to[Rerunnable] should already work.

For Future the situation is a bit more complicated. We cannot piggyback on Async[Future] as it doesn't exist and one of the laws is that the type may not memoize its result but (to my knowledge) Future does memoize. That's one of the reasons why Rerunnable exists.

Now, we could just implement LiftIO[Future] and ignore Async[Future]. There are pros and cons to it, let's go though some that came to my mind:

Cons:

  • IO[A] is lazy, but Future[A] is eager. So the implementation would have to start evaluating the computation, which is a bit more than a simple type-conversion. In your example this is made clear by the use of .unsafeRunAsync. This might not be a dealbreaker, as you'll see further below.
  • Depending on whether we need to consider any thread pool shifting the LiftIO[Future] instance might have a depenency on something like ContextShift[Future], which doesn't exist (yet). This might also not be a dealbreaker, just complicates things a bit more.

Pros:

  • People who work a great deal inside of applications based on Twitter libraries like Finagle might want to use something that's based on IO[A], so an instance of LiftIO[Future] would provide a default conversion implementation that's likely more robust than any ad-hoc implementation they'd otherwise construct. I'm not being condescending here, I'm one of those people (and we have dozens of those conversion functions lying around in the codebase).

I'd like to wait for comments from the cats-effect folks before a final conclusion but currently I think that the practical benefit of having a default conversion implementation (at least for the direction IO[A] => Future[A]) outweighs the cons. If an application is written in terms of Future[A] there is no expectation that it is referentially transparent, so losing the laziness semantics is not a huge deal in that case. Running an IO[A] on a future-pool is a bit iffy but I also think this is not a huge problem in practice. I want to do some more research on that but I have a feeling we don't need the context shifting here.

felixbr avatar Apr 12 '20 11:04 felixbr

After some discussion on gitter (link) I'd suggest we start with a simple conversion function "unsafeAsyncToFuture" which takes an F[_]: Async and turns it into a Future.

We can document why this function can be unsafe (regarding laziness and thread pool shifting) and what it should be used for. The "unsafe" prefix will hopefully be enough to stop careless usage.

This should be something that works for the tagless-final use-case you described in gitter, @bpholt. Once you verified that it solves your problem, I'd be happy to add it to the project (alongside futureToAsync and the like). What do you think?

felixbr avatar Apr 13 '20 08:04 felixbr

Yep, that makes sense to me, except that I think it may need to be Effect instead of Async? This seems to work in place of the LiftIO[Future] I suggested earlier, but I don't see how it can be implemented with only F[_] : Async.

def unsafeEffectToFuture[F[_] : Effect, A](fa: F[A]): Future[A] = {
  val p = Promise[A]

  Effect[F].runAsync(fa) {
    case Left(ex) => IO(p.setException(ex))
    case Right(x) => IO(p.setValue(x))
  }.unsafeRunSync()

  p
}

bpholt avatar Apr 13 '20 21:04 bpholt

Yeah, you're right. I wrote Async because that's what we use for the other direction (e.g. futureToAsync).

felixbr avatar Apr 14 '20 08:04 felixbr