Make timeout/timeoutTo always return the outcome of the effect

[biochimia]

timeout* methods are implemented in terms of a race between a desired effect and the timeout. In the case that both effects complete simultaneously, it could happen that the timeout would win the race, a TimeoutException be raised, and the outcome of the desired effect lost.

As is noted in #3456, this is a general problem with the race* methods, and can't be addressed in the general case without breaking the current interfaces.

This change is a more narrow take on the problem specifically focusing on the timeout and timeoutTo methods. As these methods inherently wait for both racing effects to complete, the implementation is changed to always take into account the outcome of the desired effect, only raising a TimeoutException if the timeout won the race and the desired effect was effectively canceled. Similarly, errors from the desired effect are preferentially propagated over the generic TimeoutException.

The timeoutAndForget methods are left unchanged, as they explicitly avoid waiting for the losing effect to finish.

This change allows for timeout and timeoutTo methods to be safely used on effects that acquire resources, such as Semaphore.acquire, ensuring that successful outcomes are always propagated back to the user.

[durban]

Can we write a unittest for this? I realize that it's not easy due to the race, but maybe it's possible?

A general comment about the F.canceled *> F.never pattern (I realize that race already has it, so this is not strictly about this PR): is this a case of "it can never happen (unless CE has a bug)"? If so, would it be better to raiseError instead of never?

[durban]

Could the oc.fold here be oc.embed instead?

[biochimia]

oc.embed does not apply here because the return type of IO.timeoutTo is IO[A2], and not IO[A], and is thus incompatible with Outcome.embed.

Note that the signature of timeoutTo in IO differs from that in GenTemporal. The latter returns F[A], keeping the same effect type.

[armanbilge]

Can we write a unittest for this? I realize that it's not easy due to the race, but maybe it's possible?

I think something like IO.sleep(2.seconds).uncancelable.timeout(1.second) could work.

[biochimia]

Can we write a unittest for this? I realize that it's not easy due to the race, but maybe it's possible?

I added a couple of tests in IOSpec, making use of @armanbilge's suggestion.

I'm not sure if the tests should go elsewhere. I'm also not sure of where would be appropriate place to specifically test the GenTemporal implementation.

A general comment about the F.canceled *> F.never pattern (I realize that race already has it, so this is not strictly about this PR): is this a case of "it can never happen (unless CE has a bug)"? If so, would it be better to raiseError instead of never?

On this one, I tried to follow existing practice. I don't have an informed opinion of which alternative is better. If this were to be changed, I suppose it should be done separately and cover the different places the pattern is used in, to keep the codebase consistent.

Other than unit tests. If there's alignment on moving these changes forward, I can also update the documentation to make explicit the semantics, namely with regards to the interaction between timeout* methods and uncancelable effects.

[biochimia]

oc.embed does not apply here because the return type of IO.timeoutTo is IO[A2], and not IO[A], and is thus incompatible with Outcome.embed.

Note that the signature of timeoutTo in IO differs from that in GenTemporal. The latter returns F[A], keeping the same effect type.

[armanbilge]

I'm not sure if the tests should go elsewhere.

You could add them here as well.

cats-effect/laws/shared/src/test/scala/cats/effect/laws/GenTemporalSpec.scala

Line 31 in e8669bc

class GenTemporalSpec extends Specification { outer =>

[biochimia]

Are there any guidelines on what sort of tests should go under laws/ and which should go under tests/?

For instance, I notice that the IOSpec tests include support for different "runners" (i.e., real vs ticked), while this is missing in the GenTemporalSpec that lives under laws/.

[armanbilge]

real and ticked are the "real" and "test" runtimes for IO, specifically.

while this is missing in the GenTemporalSpec

That's because these tests are not run in IO. These tests are run with PureConc "pure concurrent" transformed with TimeT which doesn't have a runtime and is designed primarily for pure testing, without side-effects.

Are there any guidelines on what sort of tests should go under laws/ and which should go under tests/?

If it is not testing something specific to IO or the runtime, then I prefer to test it with PureConc.

[biochimia]

I've added more tests under GenTemporalSpec. I also tried to add tests for cancelation semantics, but I seem to get weird semantics in the tests:

• I tried using onCancel to observe the cancellation of a timed out action, but the timeout gets triggered without the finaliser being invoked.
• I tried using F.canceled so as to test the behavior with regards to self-cancellation, but that also seemed to get ignored in the tests, triggering a timeout anyway. This one, I also tried to add with the help of .pendingUntilFixed, but the cancelled action seems to propagate and the test ends up generating an error, anyway.

These seem to be artifacts of the test setup, as I didn't observe this behaviour in the test cases I originally had for the issue.

[armanbilge]

• I tried using onCancel to observe the cancellation of a timed out action, but the timeout gets triggered without the finaliser being invoked.

Erm, there is this outstanding issue 😳

• PureConc not running finalizers #3430

[biochimia]

I've updated docs for timeout and timeoutTo methods, to reflect the updated semantics. Tests were previously added. Any feedback is welcome.

How do we move forward on this?

[djspiewak]

Let's not block this on #3430. Ideally I would like to, but fixing that has been a multi-tiered yak ziggurat, so better to move forward on this one independently.