Skip to content
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

Interruption with/without finalizer #950

Open
durban opened this issue Jun 3, 2019 · 25 comments

Comments

Projects
None yet
4 participants
@durban
Copy link

commented Jun 3, 2019

Interruption seems to behave unintuitively depending on the presence/absence of a finalizer.

Without finalizer, this never terminates (only prints "started"):

  val myTask: ZIO[Any, Nothing, Unit] = for {
    fib <- (ZIO.never.uninterruptible).fork
    _ <- ZIO.effectTotal(println("started"))
    _ <- fib.interrupt
    _ <- ZIO.effectTotal(println("cancelled"))
  } yield ()

If we add a finalizer to the forked task, it becomes cancellable (prints all 3 lines and terminates):

  val myTask: ZIO[Any, Nothing, Unit] = for {
    fib <- (ZIO.never.uninterruptible).ensuring(ZIO.effectTotal(println("finalizer"))).fork
    _ <- ZIO.effectTotal(println("started"))
    _ <- fib.interrupt
    _ <- ZIO.effectTotal(println("cancelled"))
  } yield ()

Is this intentional?

@iravid

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Can you add a latch to the second test to ensure that the fiber is actually started?

@iravid

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

e.g.

p <- Promise.make[Nothing, Unit]
fib <- ((p.succeed(()) *> ZIO.never).uninterruptible)...
...
_ <- p.await
_ <- fib.interrupt
@durban

This comment has been minimized.

Copy link
Author

commented Jun 3, 2019

Oh, you're right, sorry. With a latch, neither one terminates. I guess that's expected, right?

@iravid

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

It matches my expectations :-) Does it match yours?

@iravid

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

I don't know if we have a section about fiber interruption in the docs, but if we do, it'd be great to add a note about this behavior. It comes up all the time and is pretty subtle.

@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

@durban Yes, non-termination is correct. The never cannot be interrupted, so as long as the never begins execution (which the latch ensures), interrupt will suspend forever.

@durban

This comment has been minimized.

Copy link
Author

commented Jun 3, 2019

Ok, thanks, it makes sense.

One question: if I have a task which may or may not be interruptible, how can I make a "best effort" cancellation? (E.g., after a timeout I definitely want to continue with something else, and cancel the task if possible.) Simply fork the .interrupt call? (I.e., _ <- fib.interrupt.fork?)

Thanks for your help.

@durban durban closed this Jun 3, 2019

@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Yes, .interrupt.fork works great. Fibers don't actually consume any threads when inactive and will be garbage collected, so just fork the effects you don't care about. 👍

@durban

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

I'm sorry, but thinking about this some more, something is still strange:

  val myTask: ZIO[Any, Nothing, Nothing] =
    (ZIO.never).uninterruptible

  val mainTask: ZIO[Any, Nothing, Int] = for {
    fiber <- myTask.ensuring(ZIO.effectTotal(println("FINALIZER"))).fork
    _ <- ZIO.effectTotal(println("started"))
    _ <- fiber.interrupt
    _ <- ZIO.effectTotal(println("cancelled"))
  } yield 42

As expected, this code has a race condition (there is no latch), and sometimes doesn't terminate. However, sometimes it terminates, and prints this:

started
FINALIZER
cancelled

From this, I conclude the following:

  • We have a task (myTask), which can never be interrupted (since it's uninterruptible).
  • Since "cancelled" is printed, the previous task (fiber.interrupt) finished successfully.
  • Which means, that myTask never actually started (since if it would've started, it could never be interrupted).
  • Which means, that (since "FINALIZER" is printed) a finalizer is executed for a task which never even started. (I realize, that the documentation of ensuring says nothing about the case when the task is not started. It still seems really strange to execute the finalizer in this case.)

Am I misunderstanding something?

@durban durban reopened this Jun 7, 2019

@kaishh

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@durban A fiber can be interrupted between .ensuring and the inner action starting. That seems consistent.

@durban

This comment has been minimized.

Copy link
Author

commented Jun 8, 2019

I realize why it might be implemented this way, and that it might be considered technically correct behavior. However, why would we want it to behave this way? Why would we ever want the finalizer to execute for a task which never started? Intuitively a finalizer is something which happens after the task. Sure, for the implementation it's necessary to do some work before; but if possible, that should remain an invisible implementation detail.

So, in my opinion, it would improve the API if the finalizer would not execute in this case. (Of course it might not be feasible to implement it that way; I'm not familiar with the internals.)

@iravid

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

The sequence above surprises me as well. There are two things that might have gone wrong here:

  • The finalizer has been executed, that means that def myTask = ZIO.never.uninterruptible has started executing. But it shouldn't have been possible to interrupt myTask.
  • myTask never started executing, but the finalizer was executed anyway.

Both of these seem off to me.

@kaishh

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

@durban @iravid
I understand and agree, in fact. It is, however, documented (on ensuring scaladoc, albeit may be worded better) that ensuring itself is unsuitable for finalization for exactly that reason. I wonder if this is reproducible with bracket – that IMHO would be far more worrying.

@iravid

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

@kaishh which one of the above do you think happened? the second bullet?

@kaishh

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

@iravid I believe it interrupted exactly after ensuring executed, when going into the next iteration, after setting curIO = myTask, but before executing it. The check for interruption happens at the start of the loop, at that moment curIO is replaced by curIO = IO.interrupt and myTask is lost.

@kaishh

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

Setting temporary uninterruptibility between ensuring and next instruction there would require adding a new flag in the loop – likely worth it, actually, from usability point of view, but we'd probably want an opinion from @jdegoes on that.

@durban

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

@kaishh The behavior seems to be the same with bracket_:

  val mainTask2: ZIO[Any, Nothing, Int] = for {
    fiber <- ZIO.unit.bracket_(ZIO.effectTotal(println("FINALIZER")), myTask).fork
    _ <- ZIO.effectTotal(println("started"))
    _ <- fiber.interrupt
    _ <- ZIO.effectTotal(println("cancelled"))
  } yield 42

As far as I can tell, the code above should be semantically the same as the one with ensuring. And it also prints this:

started
FINALIZER
cancelled
@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

The guarantees provided by bracket and ensuring are quite different:

  1. Bracket provides the guarantee that release will run if acquire finishes.
  2. Ensuring provides the guarantee that the finalizer will run if the effect is started.

Note that IO.unit.bracket_(finalizer)(effect) is the same as effect ensuring finalizer.

As per this guarantee, effect.ensuring(finalizer) may run finalizer even if an "arbitrarily small" part of effect has been executed.

Any code relying on a "sufficient amount" of effect to be executed before finalization is already broken and should be refactored to use bracket, with the "sufficient amount" pulled out into an acquire effect.

Even if we were to change ensuring to run only if at least one "operation" of effect had been executed, it really wouldn't change the situation, because you would have things like:

(IO.unit *> otherEffect).ensuring(finalizer)

i.e. any "useless" or just boilerplatey machinery at the start of an effect would permit its finalizer to run after (near-immediate) interruption of the effect. Such code is finicky to refactoring and is best avoided.

There is no safe way to use ensuring for resources, except by building other primitives, which you don't need to do since we already have bracket and uninterruptibleMask.

@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Note there is a difference between:

ZIO.never.uninterruptible.ensuring(finalizer)

and:

ZIO.never.ensuring(finalizer).uninterruptible

You would need some form of "fusion" in order to guarantee equivalence of these... which I did suggest (in one form) but folks weren't a fan of that.

Ultimately, in this case, though, I think ensuring is just too low-level. It should only ever be used for installing finalizers on effects when those finalizers do not depend on running any "specific amount" of the effects. That's the only robust way to use it, which is a fine enough distinction I don't think most users should go near ensuring. Better disclaimers are required on the docs.

@kaishh

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

@jdegoes Thanks for the detailed explanation!

@iravid

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Thanks @jdegoes!

@jdegoes

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

No comments / debates / discussion? 😆

@durban

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

As per this guarantee, effect.ensuring(finalizer) may run finalizer even if an "arbitrarily small" part of effect has been executed.

@jdegoes Does that "arbitrarily small" include zero? Or is it "arbitrarily small but nonzero"?

@iravid

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Not from me. The part that was missing for my understanding was the “arbitrarily small” part.

Also, the dot notation here is misleading: the instructions are executed from bottom to top, so like Kai said it gets interrupted after installing the finalizer.

@kaishh

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

We can remove the suffix form of ensuring, if we truly believe that it's harmful 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.