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

Type-class hierarchy for cancelation #134

Merged
merged 38 commits into from Mar 6, 2018

Conversation

@alexandru
Member

alexandru commented Mar 2, 2018

This PR is the second proposal of a type class hierarchy that supports cancellation, following the development of the cancelable cats.effect.IO in #121

This proposal introduces two new and separate type classes, Concurrent and ConcurrentEffect, in addition to the existing Async and Effect.


Also see the first proposal, from #126, that modifies Async and Effect, thus making cancellation optional (as an optimization that the data type may, or may not support).


The new hierarchy:

image

The new Concurrent type class:

trait Concurrent[F[_]] extends Async[F] {

  def cancelable[A](k: (Either[Throwable, A] => Unit) => IO[Unit]): F[A]

  def uncancelable[A](fa: F[A]): F[A]

  def onCancelRaiseError[A](fa: F[A], e: Throwable): F[A]

  def start[A](fa: F[A]): F[Fiber[F, A]]

}

Details about these operations:

  1. cancelable is the builder we need, as an alternative to async, for specifying cancelation logic (in the returned IO[Unit], that being the cancelation logic suspended with IO)
  2. uncancelable undoes what cancelable is doing, thus nullifying any specified cancelation logic, yielding an F value that's atomic, i.e. either all of it executes, or none of it
  3. onCancelRaiseError materialises cancelation with a specified error, also cutting the connection between the producer and the consumer even for atomic (non-cancelable) tasks — this operation is needed for implementing logic like bracket
  4. start will start the evaluation, suspended in the F context, yielding a Fiber that can be joined or canceled

Note that I've expressed start in Concurrent as well. I no longer wish to separate them without a clear use-case — like in the FlatMap versus Monad example, there is a clear use-case for FlatMap.

Also, in comparison with the proposal in #126 there is now a much stronger law that requires that the cancellation logic gets triggered on cancellation:

  def asyncCancelableReceivesCancelSignal[A](a: A, f: (A, A) => A) = {
    val lh = F.suspend {
      var effect = a
      val async = F.cancelable[Unit](_ => IO { effect = f(effect, a) })
      F.start(async).flatMap(_.cancel).map(_ => effect)
    }
    lh <-> F.pure(f(a, a))
  }

Our LTask implementation in #126 would fail this law. So if you have cancellation logic specified in the cancelable builder, then now its usage is required when cancel is triggered (and by extension by the token returned via runCancelable).

And then the ConcurrentEffect type class is obviously:

trait ConcurrentEffect[F[_]] extends Concurrent[F] with Effect[F] {

  def runCancelable[A](fa: F[A])(cb: Either[Throwable, A] => IO[Unit]): IO[IO[Unit]]
}

Missing

N.B. we are missing a race operation in this type class hierarchy and we are missing it on IO as well. But I think we can add that in a later PR if we agree on this one.

Also Bracket is described in #113 and not part of this PR.


The Case for Separating Async from Concurrent

Given in a comment below, that I'm copy pasting here ...

The laws do not guarantee that any value is cancelable, but it does guarantee that the cancelation signal is sent and this is enough for the following.

Suppose you want to describe this operation, which is covered by our new Timer:

def sleep(d: FiniteDuration): F[Unit]

This would get implemented by ScheduledExecutorService.schedule or by JavaScript's setTimeout. But in both those cases you get a cancelable token that can be used to cancel that scheduled tick in race conditions.

Suppose we do this:

val timeout = Timer[IO].sleep(10.seconds) *> IO.raiseError(new TimeoutException)

// We don't have race yet, but we will ;-)
IO.race(request, timeout)

In my experience, most of the time the happy path will win, otherwise there's something wrong with the system. But in this case if that F[Unit] is not cancelable, this creates a resource leak, as the underlying scheduler will end up executing a lot of Runnable objects for nothing.

In the past I worked on such a system that was using Scala's Future and I did something similar to the above, with the system crashing because it was receiving over 10,000 requests per second, with the scheduler choking on those timeouts. It's also why there's a Future.timeout described in Monix, that is specialized for this specific use-case (instead of using a generic Future.race) and what motivated me to work on a cancelable Monix Task.

So I think I would be fine with #126, however we need to be clear that the signature above is unsafe. The safe version for a non-cancelable F is this:

def sleep(d: FiniteDuration): F[(F[Unit], F[Unit])]

So you need one F[Unit] for waiting on the delayed tick and another F[Unit] for triggering a cancellation. And if you'll notice, this is effectively the shape of our new Fiber, which is nothing more than this tuple.

Speaking in terms of type signatures, with a Concurrent any F[A] is actually an F[Fiber[F, A]] (F[(F[A], F[Unit])]) under the hood, but that is not true with a plain Async data type and so when you receive an F[A], its cancelation token is lost and cannot be recovered.

Therefore if we want to provide a safe sleep operation, we're talking of two distinct signatures, therefore this logic cannot be implemented with a Priority:

def sleep(d: FiniteDuration)(implicit F: Concurrent[F]): F[Unit]

def sleep(d: FiniteDuration)(implicit F: Async[F]): F[(F[Unit], F[Unit])]

Also note that we can find this signature in FS2 actually, in Promise.cancelableGet, a signature that wouldn't be necessary if F is cancelable.


Backwards Compatibility Breakage - Analysis

I've ran MiMa against version 0.9 (the currently deployed version), so measuring everything related to the new cancelable IO, including Timer, to know what to expect.

I then fixed the incompatibilities. At this point we have this list for version 0.9:

// Not a problem: AsyncInstances is a private class, we just moved those
// directly in the companion object.
//
// Manually checked:
//  - catsEitherTAsync
//  - catsOptionTAsync
//  - catsStateTAsync
//  - catsWriterTAsync
exclude[MissingClassProblem]("cats.effect.AsyncInstances"),
// Not a problem: AsyncInstances is a private class, WriterTAsync too
exclude[MissingClassProblem]("cats.effect.AsyncInstances$WriterTAsync"),
// Not a problem: AsyncInstances is a private class, OptionTAsync too
exclude[MissingClassProblem]("cats.effect.AsyncInstances$OptionTAsync"),
// Not a problem: AsyncInstances is a private class, EitherTAsync too
exclude[MissingClassProblem]("cats.effect.AsyncInstances$EitherTAsync"),
// Not a problem: AsyncInstances is a private class, StateTAsync too
exclude[MissingClassProblem]("cats.effect.AsyncInstances$StateTAsync"),
//
// Not a problem: EffectInstances is a private class and we just moved
// those in the companion object.
//
// Manual check for:
//  - catsEitherTEffect
//  - catsStateTEffect
//  - catsWriterTEffect
//  - catsWriterTEffect
exclude[MissingClassProblem]("cats.effect.EffectInstances"),
// Not a problem: Missing private traits being inherited
exclude[MissingTypesProblem]("cats.effect.Effect$"),
//
// Not a problem: SyncInstances is a private class, we just moved those
// directly in the companion object.
//
// Manual check for:
//   - catsEitherTEvalSync
//   - catsEitherTSync
//   - catsOptionTSync
//   - catsStateTSync
//   - catsWriterTSync
exclude[MissingClassProblem]("cats.effect.SyncInstances"),
// Not a problem: no longer implementing private traits
exclude[MissingTypesProblem]("cats.effect.Sync$"),
// Not a problem: SyncInstances and StateTSync are private.
exclude[MissingClassProblem]("cats.effect.SyncInstances$StateTSync"),
// Not a problem: SyncInstances and OptionTSync
exclude[MissingClassProblem]("cats.effect.SyncInstances$OptionTSync"),
// Not a problem: SyncInstances and EitherTSync
exclude[MissingClassProblem]("cats.effect.SyncInstances$EitherTSync"),
// Not a problem: SyncInstances and WriterTSync are private
exclude[MissingClassProblem]("cats.effect.SyncInstances$WriterTSync"),
//
// Not a problem: LiftIOInstances is a private class, we just moved
// those directly in the companion object.
//
// Manual check for:
//   - catsEitherTLiftIO
//   - catsKleisliLiftIO
//   - catsOptionTLiftIO
//   - catsStateTLiftIO
//   - catsWriterTLiftIO
exclude[MissingTypesProblem]("cats.effect.LiftIO$"),
exclude[MissingClassProblem]("cats.effect.LiftIOInstances"),
exclude[MissingClassProblem]("cats.effect.LiftIOInstances$OptionTLiftIO"),
exclude[MissingClassProblem]("cats.effect.LiftIOInstances$KleisliLiftIO"),
exclude[MissingClassProblem]("cats.effect.LiftIOInstances$EitherTLiftIO"),
exclude[MissingClassProblem]("cats.effect.LiftIOInstances$StateTLiftIO"),
exclude[MissingClassProblem]("cats.effect.LiftIOInstances$WriterTLiftIO"),
//
// Following are all internal implementation details:
//
// Not a problem: IO.Async is a private class
exclude[IncompatibleMethTypeProblem]("cats.effect.IO#Async.apply"),
// Not a problem: IO.Async is a private class
exclude[MissingTypesProblem]("cats.effect.Async$"),
// Not a problem: IO.Async is a private class
exclude[IncompatibleResultTypeProblem]("cats.effect.IO#Async.k"),
// Not a problem: IO.Async is a private class
exclude[IncompatibleMethTypeProblem]("cats.effect.IO#Async.copy"),
// Not a problem: IO.Async is a private class
exclude[IncompatibleResultTypeProblem]("cats.effect.IO#Async.copy$default$1"),
// Not a problem: IO.Async is a private class
exclude[IncompatibleMethTypeProblem]("cats.effect.IO#Async.this"),
// Not a problem: RestartCallback is a private class
exclude[DirectMissingMethodProblem]("cats.effect.internals.IORunLoop#RestartCallback.this"),
// Not a problem: IOPlatform is private
exclude[DirectMissingMethodProblem]("cats.effect.internals.IOPlatform.onceOnly"),
// Not a problem: IORunLoop is private
exclude[MissingClassProblem]("cats.effect.internals.IORunLoop$RestartCallback$")

The generated incompatibilities are generated by getting rid of *Instances private classes, so migrating from object Async extends AsyncInstances to just object Async.

This was unfortunately needed because the compiler crashes due to those instances being expressed in classes, possibly in combination with the SI-2712 fix 😐

I am not sure if this doesn't break binary compatibility. As an educated guess, it shouldn't and it's also the kind of work we did in the 0.8 to 0.9 migration, but MiMa cannot tell us that - we'll need to double check.

alexandru added some commits Feb 24, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Mar 2, 2018

Codecov Report

Merging #134 into master will increase coverage by 1.59%.
The diff coverage is 82.65%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   87.55%   89.14%   +1.59%     
==========================================
  Files          34       44      +10     
  Lines         691      811     +120     
  Branches       65       59       -6     
==========================================
+ Hits          605      723     +118     
- Misses         86       88       +2

@alexandru alexandru added this to the 0.10 milestone Mar 2, 2018

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Mar 2, 2018

The thing I'm not sure about is the separate hierarchy for Cancellable things

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 2, 2018

@SystemFw so you like #126 better? Could you toy with the idea please, in the context of FS2 for example and tell me which one you like best?

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Mar 2, 2018

First of all, I have to agree with you that this is really hard to get right :)
So I'm more sure about which questions I want to ask, than about answers I'm ready to give. In this case, the question is: what does a non cancellable hierarchy bring to the table, to offset the greater number of concepts?

I'll look at both proposals in more detail, but it's a very busy time unfortunately :(
My first impression is that #126 with start and cancellable merged into one might be enough, but I have not given it enough thought to be sure.

In the meantime, I think a big thanks is in order, given the incredible amount of work you're doing (particularly the "boring" bits like scaladoc, which looks awesome)

@rossabaker

This comment has been minimized.

Member

rossabaker commented Mar 2, 2018

Agree wholeheartedly on the thanks. @alexandru is doing great work here.

A guarantee that the cancelation action is triggered does not guarantee that the effect is canceled before it runs. Since it's a best effort, I still don't see why I would ever ask for a CAsync over a Priority[CAsync, Async]. And if real world use always demands the latter, it seems much simpler to put it on Async.

I guess I'd like to see an example of why I care that it tried to cancel, even if it couldn't, to prefer this over #126.

@alexandru alexandru changed the title from Type-class hierarchy for cancellation, proposal #2 to Type-class hierarchy for cancelation Mar 5, 2018

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 5, 2018

In the description of this PR I have included a "Backwards Compatibility Breakage - Analysis".

The really cool thing about the changes we're preparing for 0.10 is that the API is still source compatible and if we tried really hard we could even maintain binary compatibility!

But this is a good time to do some cleanup, so some obvious changes were made. See the details in the description.

@johnynek

This design looks really good. Thanks for working so hard to get this right.

*
* // For triggering asynchronous execution
* val cancel = io.unsafeRunSync
* // For cancellation

This comment has been minimized.

@johnynek

johnynek Mar 5, 2018

What about an example without unsafeRunSync? For instance, an application that is a single IO[Unit] value that is triggered run in a main method. It might be nicer to show an example where this is run or canceled without calling unsafe.

@@ -404,7 +439,7 @@ private[effect] abstract class IOInstances extends IOLowPriorityInstances {
par(IO.unit)
}

implicit val ioEffect: Effect[IO] = new Effect[IO] {
implicit val ioEffect: ConcurrentEffect[IO] = new ConcurrentEffect[IO] {

This comment has been minimized.

@LukaJCB

LukaJCB Mar 5, 2018

Collaborator

Tiny nitpick, maybe rename to ioConcurrentEffect?

This comment has been minimized.

@alexandru

alexandru Mar 5, 2018

Member

TBH I would rather not. That name can be considered to come from cats.effect.

Renaming it breaks binary compatibility. Not such a problem right now, but we need a stable name and we can't keep changing it depending on what type classes IO implements.

So it's either ioEffect (from cats.effect) or something else. Naming is hard, but I'd rather not have this ioConcurrentEffect.

This comment has been minimized.

@LukaJCB

LukaJCB Mar 5, 2018

Collaborator

Ah yes, that's a good reason, thanks! 👍

@@ -90,6 +90,11 @@ object arbitrary {
f2 <- getArbitrary[A => A]
} yield ioa.map(f1).map(f2)

implicit def catsEffectLawsCogenForIO[A](implicit cgfa: Cogen[Future[A]]): Cogen[IO[A]] =
cgfa.contramap((ioa: IO[A]) => ioa.unsafeToFuture())
implicit def catsEffectLawsCogenForIO[A, B](implicit cga: Cogen[A]): Cogen[IO[A]] =

This comment has been minimized.

@LukaJCB

LukaJCB Mar 5, 2018

Collaborator

Big 👍

* cancel.unsafeRunSync
* }}}
*/
def runCancelable[A](fa: F[A])(cb: Either[Throwable, A] => IO[Unit]): IO[IO[Unit]]

This comment has been minimized.

@LukaJCB

LukaJCB Mar 5, 2018

Collaborator

I like the nested IO, but it doesn't exactly signal that the inner type means cancelation. This is a pretty low level API so I think it's perfectly fine, but if someone e.g. calls flatten on it, it will immediately run and cancel the execution, right? I sort of prefer the CancelableFuture that Monix provides, but I still think this API is good enough for what we set out to do. But maybe someone has a good idea that nobody's thought about yet, so I just wanted to provide some food for thought. :)

This comment has been minimized.

@alexandru

alexandru Mar 5, 2018

Member

I like Monix's approach too, but there we have the luxury of an entire sub-project supporting it (monix-execution), whereas cats-effect is more minimal.

I think IO[Unit] is reasonable though. Once you get accustomed to what cancellation looks like, there's not much that an IO[IO[Unit]] can mean.

This comment has been minimized.

@LukaJCB

LukaJCB Mar 5, 2018

Collaborator

Yeah, agreed :)

@LukaJCB

LukaJCB approved these changes Mar 5, 2018

Excellent work, left a couple of minor comments, but overall I think this could be merged as is!

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 5, 2018

Thanks a lot for the review guys, going to torment you with another commit unfortunately: 076c409

I've added to Concurrent the missing pieces for being able to express bracket, or other similar resource handling logic:

  • uncancelable
  • onCancelRaiseError

The reasoning on uncancelable is that once you have a mechanism for specifying cancelation logic, you also need a mechanism for nullifying it. The idea is that once you have an IO, sometimes you really want that IO to be an atomic operation, so either all of it executes, or none of it.

And onCancelRaiseError is also an extension needed for expressing bracket basically. This is a mechanism for materialising cancellation. In other implementations, like Java's CompletableFuture or Haskell's async exceptions, tasks end up being completed in error when canceled. The mechanism I've chosen actually splits this part in two and it's a little different. Lighter and probably more flexible. But this second part is needed for being able to actually detect cancelation, otherwise we can't implement something like bracket.

Both these operations were already added in #121 on IO.

The good news is that by adding laws in Concurrent, I've spotted a problem in the implementation of IO#start, when used in combination with onCancelRaiseError, which is now fixed.


If I'm getting thumbs up from you again, I'd like to have this merged tomorrow night, which would be ideal, as I've got some more free time this week and would like to focus on describing race too.

@LukaJCB

This comment has been minimized.

Collaborator

LukaJCB commented Mar 5, 2018

Haha, reading your code is never a torment! ❤️

I'm not quite sure if I understood your explanation for onCancelRaiseError being needed for bracket, I understand the uncancelable part, but maybe I'll get a better grasp of it once I dive into the code!

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 5, 2018

In order to implement something like bracket, you need to materialise cancelation somehow. And onCancelRaiseError makes this possible, e.g. gave this sample in the ScalaDoc:

import scala.concurrent.CancellationException
// Reusable instance
val wasCanceled = new CancellationException()

F.onCancelRaiseError(fa, wasCanceled).attempt.flatMap {
  case Right(a) =>
    F.delay(println(s"Success: $a"))
  case Left(`wasCanceled`) =>
    F.delay(println("Was canceled!"))
  case Left(error) =>
    F.delay(println(s"Terminated in error: $error"))
}

You can also make that canceled branch to be non-terminating after you deal with it 😉

@SystemFw

This comment has been minimized.

Collaborator

SystemFw commented Mar 5, 2018

ah, I see you what you mean.
In haskell to cancel you throw an async exception to a lightweight thread directly.
Here you put the F in a cancelled state, and the F has an operation to materialise cancellation ("throwing to itself", if you will)

alexandru added some commits Mar 5, 2018

@alexandru alexandru force-pushed the alexandru:cancel-types2 branch from 7293a8e to 17c9d33 Mar 6, 2018

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 6, 2018

@rossabaker I have addressed your binary compatibility concerns and updated the description above.

At this point we only have issues related to the private Instances classes, which I hope don't break binary compatibility — those changes were needed, as described, because those mixins were crashing the compiler 😔 plus they aren't needed anyway.

We'll have to double check, but at this point I'm assuming that the 0.10 release is going to be binary compatible (but not source compatible) with version 0.9.

I did so, as you suggested, by introducing an IOBinaryCompat that makes those definitions private[internals], such that the compiler doesn't get confused not even in Cats's own code base.

Same for Async#shift, it's now a private[effect] with a @deprecated annotation too.

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 6, 2018

I have updated the description with the MiMa filter list included now in build.sbt to prove compatibility against 0.9.

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 6, 2018

I think concerns have been addressed.

I hope nobody minds that I merge this, as I want to unblock further work on race, plus I want to release a hash version in order to test that this release is indeed binary compatible with 0.9.
Until we release 0.10 everything is revertible anyway.

Plus if I kept it open, I fear more commits will come and that's a bad idea for visibility 🙂

@alexandru alexandru merged commit ea8fbc9 into typelevel:master Mar 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gvolpe gvolpe referenced this pull request Mar 6, 2018

Merged

Microsite for Cats Effect #135

gvolpe added a commit to gvolpe/cats-effect that referenced this pull request Mar 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment