Cats-effect: ConcurrentLaws.acquireIsNotCancelable unreasonable assumption about Fiber.cancel

[alexandru]

As seen in https://github.com/scalaz/scalaz-zio/pull/267

We have this law:

  def acquireIsNotCancelable[A](a1: A, a2: A) = {
    val lh =
      for {
        mVar  <- MVar[F].of(a1)
        latch <- Deferred.uncancelable[F, Unit]
        task   = F.bracket(latch.complete(()) *> mVar.put(a2))(_ => F.never[A])(_ => F.unit)
        fiber <- F.start(task)
        _     <- latch.get
        _     <- fiber.cancel
        _     <- contextShift.shift
        _     <- mVar.take
        out   <- mVar.take
      } yield out

    lh <-> F.pure(a2)
  }

This law introduces the assumption that fiber.cancel doesn't block. In cats.effect.IO's implementation there aren't at that point any finalizers registered so cats.effect.IO doesn't block on anything. But ZIO blocks for the completion of that fiber.

Which means that we need a fix that forks the cancel operation. E.g. we could use F.start(fiber.cancel) instead. Maybe even with an extra contextShift.shift just to be sure that it is indeed concurrent.

---

Published hash version with the fix for testing purposes:

1.0.0-1182d8c

[pchlupacek]

Hm, why the IO.cancel does not block if there are no finalisers? Shouldn't this be consistent? Is hard to tell from F[A] if there are/not any finalisers registered....

[Avasil]

Slightly off-topic but I would like to be able to wait on termination (reaching cancel boundary) like in ZIO as an option if not by default.

I'd prefer it to be optional (most of the times I don't care about waiting for it and as Alex mentioned it can be dangerous) but I don't know how would that look like. It's easy to go from blocking to non-blocking. No idea how would that work the other way around

[alexandru]

Guys, such proposals usually get made on the issue or the PRs — this is a project of multiple maintainers and when such an important PR hits, I expect at least playing around with the implementation; also we talked about this behavior: https://github.com/typelevel/cats-effect/pull/305

The short answer for why is because I don't think blocking on the completion of anything on cancellation is a good idea, so I preferred to be conservative and do the minimal amount of blocking necessary.

If you believe in this approach:

1. come up with an example for which the current behavior isn't sufficient and yields problems
2. make a PR — I won't, because I don't believe in it and have other things to work on, but if you believe in a feature, I can get behind popular support

@Avasil we cannot have options in cats.effect.IO, it's against the spirit of its implementation. We can have options in monix.eval.Task, via Task.Options, but we need good reasons for introducing options and here I don't see any.

I can be convinced with a sample.

[pchlupacek]

@alexandru what exactly you mean by blocking? Semantically, or Thread?

[SystemFw]

Semantically I assume

[pchlupacek]

Yeah, just want to be sure here ....

[SystemFw]

E.g. we could use F.start(fiber.cancel) in

Wasn't this already exposed in another law as well?

[pchlupacek]

@SystemFw I am a bit scared, that even semantically, it is unpredictable whether after the .cancel the F actually stopped or not. I am ok when this is dangling async call to be stoped once the cb get completed, but other than that loop shall be always finished once the cancel finishes.

[alexandru]

Semantic vs blocking the thread is irrelevant in this context. Do you wait on interruption for whatever reason or not?

As an exercise for the reader, what should this do?

val never = IO.async(cb => ())

for {
  f <- never.start
  _ <- f.cancel
} yield ()

The run-loop is obviously not finished and this is an example of a non-terminating task, which would block cancel. Making cancel wait on anything in such cases makes absolutely no sense.

[alexandru]

But please, if you can think of any examples where the current implementation is not OK, share.

[jdegoes]

ZIO's position is the same as Haskell's Async, which was developed after many years of suffering the fallout from a killThread that returns immediately (without waiting for the thread to terminate). This means ZIO's interruption will not return until the fiber being interrupted terminates (including, of course, the running of all finalizers). This model not only makes it easier to avoid "thread leaks", because after interruption, other fibers are likely to be forked, but is also strictly more powerful, since the other semantic can be recaptured simply and elegantly, e.g. fiber.interrupt.fork.void; and it's more predictable for a developer to reason about this model since it's consistent with or without finalizers.

Async tasks are always interruptible, so you can always interrupt IO.never, IO.sleep, and so forth.

I do agree this is the wrong place for this discussion and that if someone wants the behavior to change in Cats IO, they should create a pull request with the necessary changes (it doesn't look extremely trivial).

[pchlupacek]

@alexandru, if this seem somehow over-utilizing your scare time, can you please point me where all that is described? I am merely trying to first understand how that works, and the reason why I step in is that always when I see that something behaves differently behind the scenes I want to really understand this. Essentially I do not like automagical behaviours, unless absolutely necessary.

I cannot put examples yet, as I am not sure If I follow correctly, and if I really understand nature of the problem.

As for your example, I believe this is the only case where we cannot wait on F to complete, however we shall prevent any more steps in loop when that cb is eventually invoked (in F.never it won't ever be, but we couldn't really know that right?

[pchlupacek]

As for the place on discussion, just to be clear, I missed part in documentation that says IO cancel blocks only if there are finalisers, so I kinda of wanted just to reconfirm/explain that hence current IO implementation is complex and @alexandru maintains that. One can always argue, if the bug is in law or in implementation right?

[Avasil]

@alexandru
My use case was for workshop so not very convincing but it went like this:

There are passwords to decrypt using provided decrypter which randomly fails with exception and when it fails it also silently corrupts other concurrent instances. Here's the source code.

Corrupted instance returns wrong password and in this case I wanted ~100 % correctness. To do this I had to wait for each task to terminate before I could restart Decrypter. This was necessary to prevent corner cases such as cancelled tasks failing again in the background before they were terminated which led to unexpected corruption of new ones.

I've solved it using explicit Ref and having capability to backpressure on termination would make it much simpler. Though I recognize it's not a real use case so I don't have any particularly strong argument.

[alexandru]

@pchlupacek what did you think we were talking about here: https://github.com/typelevel/cats-effect/pull/305#issuecomment-412445716

Also it can't be a bug, because it works exactly like I designed it 🙂

My opinions has been shaped by my slight involvement in defining the reactive streams spec, which ended up in Java 9's Flow and their cancel doesn't block on anything, usually because it's a bad idea.

The current implementation is the best compromise I could think of for ordering inner bracket constructs making use of async finalizers (another anti-pattern), which was the original problem.

@jdegoes our implementation is not Haskell's killThread. Also "fibers" aren't linked to threads, the implementation being totally platform agnostic. Without the forking behavior on start, which was added by @oleg-py only because it was nice to have, multiple fibers could run concurrently on the same thread. Building a Fiber does not allocate anything besides some vars for state.

If there are expensive resources being used, then they can be dealt with via proper finalizers that .cancel can block on. Including building a Deferred at the outer level and installing it as the most outer finalizer.

The current implementation can be changed easily actually ... how they say in computer science, _when you have a problem, you add another var_, well in this case we simply have to add another handle/promise/deferred to wait on and identify the right moment for its completion. You can do it in user code too:

for {
  promise <- Deferred[Unit]
  fiber <- task.guarantee(promise.complete(())).start
  // Yes, this is slightly broken, can be fixed, but you get the point
  _ <- fiber.cancel *> promise.get
} yield ()

The problem is I don't want to introduce it because I don't believe it's the right approach.

[jdegoes]

our implementation is not Haskell's killThread. Also "fibers" aren't linked to threads, the implementation being totally platform agnostic.

Being the person who brought Fiber to Scala effect systems, I'm well-aware of this. 😄 Nonetheless, actively executing fibers _do_ consume threads, and spawning more fibers while others are still actively running will lead to all manner of problems commonly described as "thread leaks".

Yes, fibers can leak too, even if they suspend on async and / or yield cooperatively!

The current implementation can be changed easily actually ...

This would be extraordinarily slow, of course.

The problem is I don't want to introduce it because I don't believe it's the right approach.

And I'm convinced it's precisely the correct approach, which is why I think your fix is exactly correct: modify the law(s) that currently require(s) it so that implementations can choose whichever behavior they want. That way users can decide. 😆

And in a hypothetical future in which cats-io is a separate project, maintainers of this project can decide on the correct implementation for Cats IO.

[alexandru]

The contributors of Cats-Effect can decide on that right now, I have no problem with my opinion being overridden, it happened multiple times already and I can change my mind.

My arguments are these ...

1. it's error prone to block on finalizers when doing interruption
2. since I introduced Task in Monix, until recently, cancellation tokens have been synchronous and no possibility for blocking on async finalizers ... nobody complained, zero complaints ... when you constrain the API, users simply learn to work with what they have and the code becomes safer precisely because they learn to not rely on async finalizers
3. there are some legitimate cases in which you want to block and some legitimate cases in which you want to order async finalizers in bracket and I fixed those ... so to make bracket work well I succumbed to popular request and it now works well

Of course, I want implementations to choose their approach. Especially in this case, I know there will be implementations that cannot block on .cancel at all due to underlying implementation restrictions (e.g. based on the reactive-streams protocol maybe?).

So as far as fixing that law is concerned, of course, that's why I opened the issue, hopefully the fix will make it in the next minor release.

[RaasAhsan]

In cats.effect.IO's implementation there aren't at that point any finalizers registered so cats.effect.IO doesn't block on anything.

I seem to be confused here. In #330 the issue was that fiber.cancel was indeed backpressured on the completion of the finalizers, and the fix was to cancel it concurrently for the law. If it isn't supposed to block, how come it was in #330?

My understanding is that the semantics of IO.bracket dictate that acquire and release are guaranteed to be executed, even in the face of cancellation. What is the effect of cancelling the fiber when the finalizer is registered and when it isn't? Does it just control whether cancellation will block on the finalizers or not

[alexandru]

I seem to be confused here. In #330 the issue was that fiber.cancel was indeed backpressured on the completion of the finalizers, and the fix was to cancel it concurrently for the law. If it isn't supposed to block, how come it was in #330?

Because that's an instance in which it has to block and we missed it, as the law was introduced before we introduced blocking in fiber.cancel.

My understanding is that the semantics of IO.bracket dictate that acquire and release are guaranteed to be executed, even in the face of cancellation.

That is true. They are.

What is the effect of cancelling the fiber when the finalizer is registered and when it isn't?

If the finalizer is registered at that point, then .cancel waits for its completion. If it isn't, then the finalizers will get cancelled when registered. The race condition is indeed managed internally.

[RaasAhsan]

If the finalizer is registered at that point, then .cancel waits for its completion. If it isn't, then the finalizers will get cancelled when registered. The race condition is indeed managed internally.

Got it. From what I've dissected from the codebase, it looks like the finalizer is registered after the acquire action is completed and before the use action is started. The fiber is cancelled while it is still in the acquire action (blocked on mVar), so the finalizer has not yet been registered. Therefore cancel completes without blocking.

If it isn't, then the finalizers will get cancelled when registered.

If this is true, when the acquire action of a bracket operation is concurrently cancelled, would the release action not be executed?

[alexandru]

If this is true, when the acquire action of a bracket operation is concurrently cancelled, would the release action not be executed?

In case of bracket, the acquire is always uncancelable and there is absolutely no instance in which an acquire gets executed without a corresponding release. The cancel operation does not prevent any of that.

---

As a more general message, because it appears this behavior is a surprise, to have a more productive conversation, please read:

1. the original issue on sequencing of finalizers in order to order the finalization of inner brackets.
2. my design rant in which I complain about start being a very error prone operation that in most cases can be substituted either with race or with parMap2 which are far safer and in which I also talk about the cancellation model of the Monix Task from back then

I.e. if it weren't for bracket, I wouldn't have made sequenced async finalizers (because async finalizers are in general an anti-pattern) and if it weren't for FS2 requesting a start and for the laws of Concurrent needing some way to trigger cancellation in order to observe it, I wouldn't have added start to the type class hierarchy at all.

More powerful is not always better. I've had zero complaints about Monix Task missing start or the capability to wait on async finalizers throughout the entire 2.x series. I've had only one complaint about Cats-Effect 0.10 and that's only because the user was confused by the signature of IO.cancelable (using IO[Unit] instead of () => Unit or a SyncIO[Unit]), which never happened with Task as Task made it clear.

Also please read the source code for the behavior, here's what's relevant:

1. IOConnection that manages the finalizers, in particular see what happens on push or cancel
2. IO.cancelable to clearly see how this is used
3. The start implementation, making use of IOConnection
4. The bracket implementation making use of IOConnection.push

The source code describes this behavior perfectly.

[alexandru]

I've thought about it while riding my bike 🙂 and actually there is an improvement we can do ... when a bracket's acquire starts executing, we can expect that a finalizer is coming so we could block fiber.cancel for that, because everything in this case is about bracket's behavior.

I don't have time for it right now, but the change is pretty straightforward.

[alexandru]

Goddammit, there, I hope you're happy 🙂

https://github.com/typelevel/cats-effect/pull/376

@pchlupacek please inspect the behavior this time, as to not be a surprise when we discuss about it the next time.

[pchlupacek]

@alexandru thank you for the fix.