onCancel + continual

[SystemFw]

This PR adds onCancel to execute an action on cancelation, and continual, an interruption safe equivalent of attempt.flatMap.

Although there is a longer discussion to be had wrt our cancelation model, in this PR I focused exclusively on implementing things in combinator land, with no internal changes: these should be usable today, by any effect. The main drawback right now is the heaviness of continual.

MiMa tells me this is fully binary compatible. ( -.-" )

Breaks bincompat on 2.11 only

[djspiewak]

Is there a more efficient implementation of this for IO, or other concrete datatypes?

[SystemFw]

There are all sorts of things you can do if you allow internal changes, at various levels of ambition. If all you want is continual, all you need to do is disable the the interrupt check on flatMap (continual is equivalent to our previous model without interruptible flatMaps, in which case it's equivalent to attempt.flatMap)

[alexandru]

We could implement it more efficiently, yes. It should eventually end up on one of our traits.

[rossabaker]

Note that Cats 2.0 is not breaking binary compatibility on 2.11. Are we all comfortable breaking with their precedent there? I lean toward yes, but I want to highlight that, since we're otherwise mirroring their approach.

[SystemFw]

I think we could avoid breaking bincompat here if we only add these operations to the syntax via bincompat traits, but then users won't be able to override them. It's not important for onCancel, but it is important for continual.

In any case it was my understanding we were going to break bincompat for 2.0, but happy to discuss :)

[oleg-py]

Why _ =>?

[SystemFw]

typo, will fix, cheers

[rossabaker]

We could split this into something that works on series/1.x either by just having the function on the Continual companion, or using a bincompat trait (which can't be overridden) to get the syntax we eventually want on the typeclass. A similar technique is possible for onCancel, which is (I think) less compelling to override.

We haven't firmly decided whether to break bincompat in 2.0 or not outside of laws. Semver says we can, alignment to cats precedent says not. If we do, it would be nice to fix a couple other major annoyances and push 3.0 deeper into the future. My inclination is to either fix our biggest warts in 2.0 or to keep 2.0 compatible. The ecosystem is going to need a cats-effect-2.0 quickly after cats-2.0, so there is some time pressure.

[djspiewak]

@rossabaker My view: I'm okay with breaking bincompat in 2.0, but only very minorly. That's a sliding scale to be sure, but I think one that most users would be okay with. For example, breaking compatibility due to adding concrete trait functions (which only breaks 2.11) is okay in my book. Cleaning up any implicit shimming (which we probably don't have, as you referenced the other night) is also okay. Basically if it maintains source compatibility, it's definitely okay in my book.

That's just my opinion. Open to being convinced.

[rossabaker]

The difference between breaking binary compatibility and not in core the difference between that downstream projects should rerelease (or have their Gitters flooded with users asking about eviction warnings) or that they must. Source compatibility is a worthy goal to lessen this burden if we impose it.

So, yeah, I'm 👍 on this PR for 2.x. The outstanding question is whether any pieces should be brought back to 1.3.

[SystemFw]

I agree with @djspiewak characterisation wrt bincompat in 2.0

The outstanding question is whether any pieces should be brought back to 1.3.

How long are we talking about for 2.0? if it's a matter of weeks it might be not worth doing all the BinCompat trait dance, since both of these things are achievable in user code. If it's a matter of months then maybe it is

[rossabaker]

Yes, a matter of weeks.

[alexandru]

This obviously breaks binary compatibility on Scala 2.11.

I'll be glad when we won't have to support Scala 2.11 anymore.

[alexandru]

Unfortunately this too breaks binary compatibility on Scala 2.11.

[alexandru]

This is a pretty cool development, I wanted something like continual for a long time.

However if we end up adding it as more than a function, I don't think it should be on Concurrent. I think it should be on Bracket and the reason for that is because we want continual in cases in which it isn't reasonable to request Concurrent as a restriction.

Consider that for data types that that don't implement Concurrent, the continual op is attempt.flatMap.

[SystemFw]

However if we end up adding it as more than a function, I don't think it should be on Concurrent. I think it should be on Bracket

I would actually be keen on adding it as a function in 2.0, however inefficient it might be, and then have a broader discussion around our cancelation model.
I have made a proposal here, which looks like

mask {
    poll { sem.acquire }.flatMap(yourThing)
}

After that, I think ZIO implemented something very similar with cancelable/uncancelable

[jdegoes]

This looks good and useful pre Cats Effect 3.0. I do think it's worth explicitly documenting that F[B] becomes uninterruptible.

When used right-associativity,

fa.flatMapCont(a => fb.flatMap(b => fc.flatMap(c => fd.flatMap(...))))

You can see that the first continual (which I have called flatMapCont) now renders the entire rest of the chain permanently uninterruptible.

This is of course less constraining than fa.flatMapCont(...).uncancelable, because it allows interruption on the first operation ("acquisition").

But the behavior of "continual" uncancelability after that first "flatMap" may be surprising and could benefit from documentation.

I would not be a fan of this operation ending up on "Cats Effect 3.0", but whatever operations end up on the table, I agree with Alex it should end up on a trait so library authors can specialize. e.g.:

def continual[A, B](fa: Task[A])(f: Either[Throwable, A] => Task[B]) = 
  ZIO.uninterruptible {
    ZIO.interruptible(task).either.flatMap(f)
  }

Finally, as I have pointed out many times now, both here and on ZIO, any attempt to make acquisition interruptible in an F[_] effect monad will break resource safety on subtle, benign refactorings.

Let's say the interruptible acquisition operation intAcquire is made up of foo, bar, baz`:

intAcquire = foo *> bar *> baz

Now let's say this is somehow safe, such that baz is an atomic operation that acquires a resource. Then currently it is safe to use intAcquire in the following scenario:

continual(intAcquire)(k => ...)

However, a subtle, benign change to intAcquire, which is allowed for by monad laws, now breaks resource safety:

intAcquire = foo *> bar *> baz <* F.pure(())

One can see that the resource safety of the code being so fragile to seemingly benign refactorings is a sign that something is deeply wrong with the model; indeed, there is a scenario with the above code that actually breaks the right identity monad law.

I briefly discussed with @alexandru and suggested an Atomic type (not a great name) without the full capabilities of F[_]; rather, Atomic would be a constructor for single-step synchronous and asynchronous effects. This allows you to provide pre-defined ways to lift Atomic into F[_], as well as describe acquisition operations in terms of Atomic, not F[_].

An alternative is to say Acquire[F[_], A](allocate: F[(F[A], F[Unit])]. That is, define an allocation data type that splits allocation into an interruptible allocation (F[A]), followed by a cleanup operation that's always run, regardless of interruption (F[Unit]). This structure is well-defined in the presence of interruption and stable in the presence of refactoring; it's how Cats Effect Semaphore achieves safe interruption of acquisition.

[djspiewak]

This is all very clever and terrifying. I look forward to when we have a more primitive implementation.

[djspiewak]

I think John's point is a good one about the subtle perils of this sort of stuff, but there's a broader underlying point beneath it, which is that intuitions about bind associativity simply do not apply in these "bind like" operators. This is one reason why I think continual is just a better name than flatMapCont: if you treat it like flatMap, you're going to be very surprised. I haven't looked, but continual almost certainly does form a second lawful Monad instance with the interruption properties you would expect, but that Monad does not compose consistently with the primary Monad formed by flatMap.

Perhaps a less abstract way of putting this would be that users should not rely on the coincidental uninterruptability of effects within a bind chain passed to continual. More precisely:

// given
continual(fa *> fb)(b => g(b))

In this chain, only the bind between fb and g(b) is guaranteed to be interruption-free. No guarantees should be assumed about fa, fb, or g(b).

[djspiewak]

@SystemFw Can you merge and correct build failures?

[codecov-io]

Codecov Report

Merging #519 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
+ Coverage   89.71%   89.81%   +0.09%     
==========================================
  Files          71       71              
  Lines        2051     2071      +20     
  Branches      140      125      -15     
==========================================
+ Hits         1840     1860      +20     
  Misses        211      211