Corrected handling of self-cancelation within timeout#3453
Corrected handling of self-cancelation within timeout#3453djspiewak merged 2 commits intotypelevel:series/3.xfrom
timeout#3453Conversation
armanbilge
left a comment
armanbilge
left a comment
There was a problem hiding this comment.
I wasn't sure about this initially, but #3456 and the linked Discord discussion changed my mind.
This also addresses the problem I encountered in #3232 (comment). Embedding the cancelation does seem like the right default, with racePair available for more customized semantics.
| poll(racePair(fa, sleep(duration))) flatMap { | ||
| case Left((oc, f)) => | ||
| poll(f.cancel *> oc.embedNever) | ||
| poll(f.cancel *> oc.embed(poll(F.canceled) *> F.never)) |
There was a problem hiding this comment.
I was just about to open a bug for the current version (3.4.8), as timeoutAndForget was uncancelable, but then saw that you already fixed it.
I'd like to ask, however, what is the meaning of this line here? And does it work on the current version?
This change is confusing. What's the difference between oc.embed(poll(F.canceled) *> F.never) and oc.embedNever? It feels like this change is needed due to some unintuitive interaction between the operations involved (race, embed and poll).
I am asking because we are migrating a big codebase from Cats-Effect v2, to Cats-Effect v3, and I need custom semantics for our timeout operations, which means our custom timeout implementation. And we need something workable right now.
Thanks for any insight you can give.
There was a problem hiding this comment.
What's the difference between
oc.embed(poll(F.canceled) *> F.never)andoc.embedNever?
oc.embedNeverwill hang indefinitely ifocis canceledoc.embed(poll(F.canceled) *> F.never)will self-cancel ifocis canceled
There was a problem hiding this comment.
Thank you kindly for your answer @armanbilge,
But what's the problem if embedNever hangs? Given it's in the context of poll, IO.never should be cancelable. This is the part that I'm missing.
There was a problem hiding this comment.
Sorry, if I'm generating noise on an issue right now, maybe this should have been asked on the forum.
I wondered if this isn't some mistake, or an API issue. But I may have gaps in how that embed / fold works. Intuitively, embedNever should have worked, but I guess it doesn't have access to that poll.
There was a problem hiding this comment.
But what's the problem if
embedNeverhangs?
Then we end up with problems like #3396.
Given it's in the context of
poll,IO.nevershould be cancelable. This is the part that I'm missing.
Right, it's cancelable, but it will hang unless it is canceled. The semantic we decided we want here is for cancelation to propagate.
FakeFiber was used by KafkaConsumer to manage: 1. the consumerActor fiber, processing requests (including polls); 2. the pollScheduler fiber, scheduling poll requests (subject to backpressure); 3. a fiber combining the above two fibers. Compared to a regular fiber, FakeFiber offered a method to combine two fibers by racing them one against the other. The semantics were similar to the race method for effects, but operating at the fiber level. In KafkaConsumer, FakeFiber was used with cancellation effects that returned immediately (i.e., fiber.cancel.start.void). In addition the fiber outcome was relayed to KafkaConsumer.awaitTermination. With this change FakeFiber is replaced with an Async[F].race of the underlying effects. This is managed in the new method startBackgroundConsumer, which builds upon effects assembled by runConsumerActor and runPollScheduler. These effects are unwrapped from the previous fibers. As before, cancellation of the consumer effect is only waited on in awaitTermination, where any errors are propagated. Compared to the original behaviour of FakeFiber.combine, starting with cats-effect 3.5.0, cancellation of one of the consumer effects will lead to cancellation of both, as per changes introduced with typelevel/cats-effect#3453. I'm not sure how cancelation would come into play here, but the behaviour change looks appropriate: without one of the racing fibers KafkaConsumer would not be functional (no polls scheduled, or no requests/polls processed).
FakeFiber was used by KafkaConsumer to manage: 1. the consumerActor fiber, processing requests (including polls); 2. the pollScheduler fiber, scheduling poll requests (subject to backpressure); 3. a fiber combining the above two fibers. Compared to a regular fiber, FakeFiber offered a method to combine two fibers by racing them one against the other. The semantics were similar to the race method for effects, but operating at the fiber level. In KafkaConsumer, FakeFiber was used with cancellation effects that returned immediately (i.e., fiber.cancel.start.void). In addition the fiber outcome was relayed to KafkaConsumer.awaitTermination. With this change FakeFiber is replaced with an Async[F].race of the underlying effects. This is managed in the new method startBackgroundConsumer, which builds upon effects assembled by runConsumerActor and runPollScheduler. These effects are unwrapped from the previous fibers. As before, cancellation of the consumer effect is only waited on in awaitTermination, where any errors are propagated. Compared to the original behaviour of FakeFiber.combine, starting with cats-effect 3.5.0, cancellation of one of the consumer effects will lead to cancellation of both, as per changes introduced with typelevel/cats-effect#3453. I'm not sure how cancelation would come into play here, but the behaviour change looks appropriate: without one of the racing fibers KafkaConsumer would not be functional (no polls scheduled, or no requests/polls processed).
3 participants
This has to be targeted at 3.5.0 since it requires a law change. In particular, this adjusts the default race semantic such that
race(canceled, fa)is nowcanceledrather than being equal tofa. I think this actually makes a lot more sense.Fixes #3396