Make `timeout`/`timeoutTo` always return the outcome of the effect

[biochimia]

trafficstars

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.

[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.

[armanbilge]

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

You could add them here as well.

https://github.com/typelevel/cats-effect/blob/e8669bc3bcffffc15ce0a9d5156ad33c5d6ac16f/laws/shared/src/test/scala/cats/effect/laws/GenTemporalSpec.scala#L31

[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 😳

• https://github.com/typelevel/cats-effect/issues/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?