Fix propagation of interruption from CIO to ZIO #696
Conversation
| .addObserver(exit => cb(Right(exit))) | ||
| val cancelerEffect = F.delay { | ||
| val _ = fiber.interrupt | ||
| out match { |
There was a problem hiding this comment.
Cats syntax is imported so you could use out.leftMap
There was a problem hiding this comment.
I thought about it but I decided against it as it's going to increase allocations from the Function1 and the closures of local vals captured within the function. I think we can afford a bit of "uglier code" to improve performance, wdut?
| case Left(fiber) => | ||
| val completeCb = (exit: Exit[Throwable, A]) => cb(Right(exit)) | ||
| fiber.unsafe.addObserver(completeCb) | ||
| Left(Some(F.async[Unit] { cb => |
There was a problem hiding this comment.
Does the finalizer need to wait for the fiber complete?
There was a problem hiding this comment.
I don't think it does. The finalizer is wrapped in an effect so it's only invoked when cancellation is invoked. Is that explaining what you asked or did I miss something?
There was a problem hiding this comment.
My understanding is that this finalizer is F.async because we want it to complete only when the ZIO fiber completes. I was wondering if it could just be F.defer and be more of a fire and forget as in:
Left(Some(F.delay {
fiber.tellInterrupt(Cause.interrupt(fiber.id))
}))
There was a problem hiding this comment.
Ahhh wait now I understood your initial comment. I think we do need to await for it, otherwise we might be leaking resources etc. Also if we didn't await for it, the test that I added would be failing because there is no guarantee that the side effect is run before the CIO completes
There was a problem hiding this comment.
How does making it async prevent resource leakage? The only way for something to leak is if the ZIO fiber ignores the interupt, which is an entirely different problem. And if that does ever happen by waiting this is now leaking a cats fiber so I think it makes it worse not better.
Also if the CIO is used in a race but the ZIO takes a while to respond to the interrupt then it is holding up the downstream effects unnecessarily. Actually it might be worth seeing whether CIO race waits or not.
As for the test, can't it add an onExit to the ZIO that completes a promise and wait for that? That's what I started doing in my repro.
There was a problem hiding this comment.
The async part is so that we don't synchronously block till interruption is completed.
Let's forget about interop for a minute. If we were working purely with ZIO or CIO, wouldn't you expect that all finalisers have finished executing when you issue an interrupt signal and await for the fiber to complete? This is what the async part does in a nutshell; it makes sure that everything in the interrupted effect has finalised by the time we resume execution.
There was a problem hiding this comment.
It's a good question. I can think of cases where I would want both. Looks like IO.race vs IO.racePair. The former will cancel looser and await its finalisation, the later will not. Similarly with ZIO.race. So yeah you are correct.
| fiber.unsafe.addObserver(interruptCb) | ||
| fiber.unsafe.removeObserver(completeCb) | ||
| fiber.tellInterrupt(Cause.interrupt(fiber.id)) | ||
| // Allow the interruption to be interrupted |
There was a problem hiding this comment.
It looks like finalizers are uncancellable so this will never happen.
There was a problem hiding this comment.
Huh thanks for pointing this out. I was debating with my own self whether to add it 😬
Only reason I added it was just in case CE adds support for timing out interruption. I'm happy to remove it though to make the code more readable. Wdut?
There was a problem hiding this comment.
I'm leaning towards removing it, especially if we don't need the finalizer to be async. It also seems unlikely that they would ever add it because it would be turtles all the way down. Could you then cancel the cancellation of the cancellation?
There was a problem hiding this comment.
Could you then cancel the cancellation of the cancellation
My thinking is that some frameworks allow you to set a deadline for cancellation. But yeah I think let's remove it since it's a bit of an overkill for something that's not supported yet
|
Closing and reopening as an attempt for the |
|
@steinybot closing and reopening didn't seem to work, I'll close it, address the comments and open it as a new PR |
/fixes #695
/claim #695
Note that I also changed the code to use
runOrForkinstead offorkwithintoEffect. This is to avoid the overhead of forking fibers when the ZIO effect can be evaluated synchronously. Users that convert simple ZIO effects to CIO often will see a very notable performance improvement from this change