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, proposal #1 #126

Closed
wants to merge 15 commits into
base: master
from

Conversation

5 participants
@alexandru
Member

alexandru commented Feb 24, 2018

This PR is the first proposal for modifying the existing type classes in order to expose operations for cancellation.


Also see the proposal in #134 that adds new type classes instead of modifying the existing ones.


Changes:

  1. modified Async to be able to express IO.cancelable
  2. adds AsyncStart to be able to express IO#start
  3. modifies Effect to inherit AsyncStart and to be able to express runCancelable
  4. adds runCancelable on IO as the safe analogue to unsafeRunCancelable
  5. removes Async#shift because it's lawless, moved to the Async companion as a simple utility (don't see any potential for optimizations depending on F)
  6. fixes #127, fixes #128, fixes #129

Diagram (accounting for Bracket in #113):

FAQ

Do we want cancellation for every Effect?

No, but the idea I'm having here is that cancellation is basically a no-op for Effect data types that are not capable of cancellation. All the provided laws thus far take this into account. And for example we don't test that the provided cancellation token is actually triggered but we do test that:

  1. triggering the token returned by runCancellable can be done synchronously (with .unsafeRunSync)
  2. start.flatMap(_.cancel) never back-pressures, being always equivalent with IO.unit
  3. the result of triggering that token on runCancellable is triggering the same side effect as start(fa).flatMap(_.cancel)

To me this is a reasonable set of laws. And even if the reference IO no longer behaves like a non-cancellable effect, in the tests suite I introduced this type, which is tested against all laws of Effect:

type LTask[+A] = ExecutionContext => Future[A]

See implementation.

PS: I found bugs #127, #128 and #129 this way 😉

How is polymorphic code impacted?

This is where it gets tricky. Normally with Monix's Task and the upcoming IO we can express this operation safely, because the returned task is cancelable and so we can cleanup resources in race conditions:

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

However, with the non-cancelable IO that we have on master, users need to choose either to:

  1. block indefinitely (e.g. current implementation of parMap2 for IO)
  2. have resource leakage (e.g. what usually happens with Scala's Future in common use by beginners)
  3. expose a cancellation token

So what you get in solutions like FS2 is actually a tuple that can be used to block on the result, or to cancel it:

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

This is actually no different than what we get with start, that result having the shape of our new Fiber:

def sleep(timespan: FiniteDuration): F[Fiber[F, Unit]]

But unfortunately we can't go from F[A] to F[Fiber[F, A]] unless F is cancelable. So polymorphic code that needs to be safe will have to return that Fiber result everywhere it matters and to bind it around in order to combine it with other fibers.

And then the problem is that race conditions, like parMap2 when one of the tasks is a failure, might choose to block for both tasks due to safety reasons, so there are instances in which, even if you have a Fiber in your hand, you can't actually use it. Ditto, you can't describe a safe IO.race without that IO being cancelable and you no longer have Fiber results that you can use.

Another problem is that a CancelableAsync <: Async probably violates Liskov's substitution principle.

Why is Async.cancelable not on AsyncStart?

Because the difference between cancelable and start is precisely the difference between async and runAsync. It's basically a different level. You can't express start in terms of cancelable.

You can express start in terms of cancelable + runCancelable + some knowledge about the data type (as can be see by the implementations of EitherT, StateT and WriterT).

And actually you can have a generic start in terms of cancelable + runCancelable, but it can lose info when it comes to these monad transformers — but that's besides the point, given that all implementations worked, that's good enough for Effect <: AsyncStart.

Why isn't AsyncStart embedded in Effect?

Because Monix's Task can express start without a Scheduler, but it needs a Scheduler for runAsync / runCancelable.

This highlights the fact that start is less powerful, sort of like a lighter version of runAsync / runCancelable.

alexandru added some commits Feb 24, 2018

@LukaJCB

I think this is amazing as usual, lots and lots of care put into it and great scaladocs as well. I can hardly thank you enough!

That said, I did leave a couple of comments, two of them very minor and I also have another more broad question that we might we want to debate.

Do we want cancellation for every Effect? This PR assumes that we do and for me personally I think this is great, but I believe there was some initial resistance.
I could envision leaving Effect as it is (adding bracket there somewhere) and adding an extra class with cancellation.

Thanks again!

}

implicit def catsWriterTEffect[F[_]: Effect, L: Monoid]: Effect[WriterT[F, L, ?]] =
new Effect[WriterT[F, L, ?]] with Async.WriterTAsync[F, L] {
new Effect[({type λ[α] = WriterT[F, L, α]})#λ] with Async.WriterTAsync[F, L] {

This comment has been minimized.

@LukaJCB

LukaJCB Feb 24, 2018

Collaborator

Why revert to type lambdas here?

This comment has been minimized.

@alexandru

alexandru Feb 24, 2018

Member

Because IntellIJ Is acting up. It's just junk, will revert.

@@ -43,6 +43,9 @@ private[effect] object Callback {
/** Reusable `Right(())` reference. */
final val rightUnit = Right(())

/** Reusable no-op, side-effectful `Function2` reference. */

This comment has been minimized.

@LukaJCB

LukaJCB Feb 24, 2018

Collaborator

You mean Function1 reference?

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

This comment has been minimized.

@LukaJCB

LukaJCB Feb 24, 2018

Collaborator

Is there a specific reason why this isn't on AsyncStart, you say in the docs that cancellation is optional, so why not just put it on AsyncStart so if people don't want to implement cancellation they don't have to deal with it.

As far as I can see the only law that touches this is asyncCancelableCoherence, which could easily also be put on AsyncStart, or am I missing something?

This comment has been minimized.

@johnynek

johnynek Feb 26, 2018

I'm not excited about cancellation being optional, but also always on Async. Can we not have another layer? Either use AsyncStart or AsyncCancelable?

This comment has been minimized.

@rossabaker

rossabaker Feb 26, 2018

Member

Is there a difference between AsyncCancelable and the CAsync in @alexandru's second diagram?

As long as a no-op cancel is a lawful, what do we gain by pushing cancelation down? We could always derive an uncancelable CAsync[F] from any Async[F]. So what function might we write constrained by CAsync where an Async wouldn't do?

The optional operation bothered me at first glance, but when I think about it acting as an optimization hint, it feels right.

This comment has been minimized.

@alexandru

alexandru Feb 26, 2018

Member

By introducing a CAsync alternative, then there will be a law that says the side effects specified in the CAsync.cancelable builder must be triggered on cancellation. Otherwise a CAsync makes no sense.

I am preparing a second PR, to have as an alternative — we can then pick and choose.

Naming is hard though. I am going to go with the C prefix for now and introduce CAsync and CEffect, but I'm open to suggestions 😀

This comment has been minimized.

@rossabaker

rossabaker Feb 26, 2018

Member

In a split hierarchy, how would I express "All I need is Async, but here's an optimization hint just in case you're cancelable"? I have not had a chance to code against this PR yet, but I imagine this being the most typical use.

This comment has been minimized.

@johnynek

johnynek Feb 26, 2018

@rossabaker have you seen the priority pattern:

https://github.com/typelevel/algebra/blob/master/core/src/main/scala/algebra/Priority.scala

you can do: implicit F: Priority[CAsync[F], Async[F]] and then handle both cases. This is not uncommon when you have optimizations for commutative groups or having a group vs a monoid...

This comment has been minimized.

@alexandru

alexandru Feb 26, 2018

Member

That pattern is really cool. We can’t depend on Algebra though, we’d have to copy/paste it because ...

We have this problem in IO’s API actually, in the def to[F[_]] op that wants an Async currently.

Will follow with a CAsync alternative since this decision shouldn’t be taken lightly, so having code to compare makes it easier to talk about it.

However my idea in this PR was that of Ross. CAsync and CEffect will come with stronger laws.

So the question then is - what would be the use-case? Could we point to some use-case where optional cancelability is a problem?

In Monix I don’t have any yet. I mean, I have for example parZip on Iterant, but the parMap implementation can be described without cancelability, as in 0.9. And I haven’t done races yet.

Races are problematic. Having a timeout in case some task doesn’t complete in time only works well with cancelation.

The problem is basically code that was doing F[(F[A], F[Unit])] and that will change to F[A] as a result of cancelability. I think there are such instances in FS2 (e.g. Promise), but wonder how important they are.

This comment has been minimized.

@rossabaker

rossabaker Feb 27, 2018

Member

Ooh, that Priority is great. Thanks for the pointer. I certainly need this elsewhere.

I still can't think of a use case that justifies a deeper hierarchy and Priority-like machinery just so we can discriminate Asyncs that may have a non-trivial cancel.

alexandru added some commits Feb 24, 2018

@alexandru

This comment has been minimized.

Member

alexandru commented Feb 24, 2018

@LukaJCB Hey, thanks for the review, I started writing a comment, then moved it in the description. Read that one 😀

alexandru added some commits Feb 25, 2018

@alexandru alexandru changed the title from WIP: New type-class hierarchy to New type-class hierarchy Feb 25, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Feb 25, 2018

Codecov Report

Merging #126 into master will increase coverage by 0.82%.
The diff coverage is 93.1%.

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage   87.55%   88.38%   +0.82%     
==========================================
  Files          34       36       +2     
  Lines         691      680      -11     
  Branches       65       64       -1     
==========================================
- Hits          605      601       -4     
+ Misses         86       79       -7

@alexandru alexandru changed the title from New type-class hierarchy to Type-class hierarchy changes Feb 25, 2018

@alexandru alexandru changed the title from Type-class hierarchy changes to Type-class hierarchy changes for cancelation Feb 25, 2018

@alexandru alexandru added this to the 0.10 milestone Feb 25, 2018

alexandru added some commits Feb 25, 2018

@johnynek

This PR is large. Would it be possible to make the copyright changes in a separate PR and allow us to focus our review with less non-functional diff?

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

This comment has been minimized.

@johnynek

johnynek Feb 26, 2018

I'm not excited about cancellation being optional, but also always on Async. Can we not have another layer? Either use AsyncStart or AsyncCancelable?

@@ -32,9 +31,44 @@ import scala.util.Either
@implicitNotFound("""Cannot find implicit value for Effect[${F}].
Building this implicit value might depend on having an implicit
s.c.ExecutionContext in scope, a Strategy or some equivalent type.""")
trait Effect[F[_]] extends Async[F] {

trait Effect[F[_]] extends AsyncStart[F] {

This comment has been minimized.

@johnynek

johnynek Feb 26, 2018

why does this need to extend AsyncStart? Why not EffectStart that extends Effect[F] with AsyncStart[F]?

@alexandru alexandru changed the base branch from next to master Feb 26, 2018

@alexandru

This comment has been minimized.

Member

alexandru commented Feb 26, 2018

@johnynek the problem with the PR is that it was not made as a comparison with master, but with another branch I created just so I can compare this PR. I just changed the base, the copyright headers are gone.

On your comments, I will create a second PR with new type classes, as an alternative to extending the existing ones.

@alexandru alexandru changed the title from Type-class hierarchy changes for cancelation to Type-class hierarchy for cancelation, proposal #1 Feb 26, 2018

alexandru added some commits Feb 26, 2018

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 2, 2018

I've created the second proposal, which adds new type classes instead of modifying the existing ones, with stronger laws as well: #134

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 2, 2018

NOTE — that second proposal does not have an AsyncStart equivalent, start being added in CAsync (the cancellable Async) ... we can do this because, imo, we have a guarantee that cancellation works and in absence of clear use-cases for separation (like in FlatMap vs Monad), this is pretty good.

Therefore my preference has shifted towards this second proposal, because it only adds one extra type-class (versus this one), while providing a clearer contract for Async vs CAsync.

And in case of optionality, we might use the "Priority" pattern, as described by @johnynek above.

@alexandru

This comment has been minimized.

Member

alexandru commented Mar 5, 2018

I've lost faith in this approach, because with this optional cancellation approach, the laws aren't strong enough to ensure safety for an eventual race operation.

As explained in #134, even if sometimes cancelation can be treated like an optimisation, when resource safety is needed in race conditions, the presence or absence of cancellability changes the signature of functions.

I'm closing this PR for the time being, to focus on the other one. This one can always be reopened, but I doubt we'll need to do it.

@alexandru alexandru closed this Mar 5, 2018

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