Make timeout/timeoutTo always return the outcome of the effect
#4059
Conversation
biochimia
force-pushed
the
timeout-returns-effect-outcome
branch
from
699e7a2
to
b4fd74c
Compare
There was a problem hiding this comment.
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?
| poll(racePair(IO.sleep(finiteDuration))) flatMap { | ||
| case Left((oc, f)) => f.cancel *> oc.embed(poll(IO.canceled) *> IO.never) | ||
| case Right((f, _)) => | ||
| f.cancel *> f.join.flatMap { oc => oc.fold(fallback, IO.raiseError, identity) } |
There was a problem hiding this comment.
Could the oc.fold here be oc.embed instead?
There was a problem hiding this comment.
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.
I think something like |
biochimia
force-pushed
the
timeout-returns-effect-outcome
branch
from
b4fd74c
to
ff6a283
Compare
There was a problem hiding this comment.
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.
| poll(racePair(IO.sleep(finiteDuration))) flatMap { | ||
| case Left((oc, f)) => f.cancel *> oc.embed(poll(IO.canceled) *> IO.never) | ||
| case Right((f, _)) => | ||
| f.cancel *> f.join.flatMap { oc => oc.fold(fallback, IO.raiseError, identity) } |
There was a problem hiding this comment.
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.
biochimia
force-pushed
the
timeout-returns-effect-outcome
branch
from
ff6a283
to
72cb7f7
Compare
You could add them here as well. |
|
Are there any guidelines on what sort of tests should go under For instance, I notice that the |
|
That's because these tests are not run in
If it is not testing something specific to |
biochimia
force-pushed
the
timeout-returns-effect-outcome
branch
2 times, most recently
from
ee7f1f6
to
4ccb572
Compare
|
I've added more tests under
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. |
Erm, there is this outstanding issue 😳 |
`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 typelevel#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.
biochimia
force-pushed
the
timeout-returns-effect-outcome
branch
from
4ccb572
to
718c78e
Compare
|
I've updated docs for How do we move forward on this? |
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, aTimeoutExceptionbe 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
timeoutandtimeoutTomethods. 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 aTimeoutExceptionif the timeout won the race and the desired effect was effectively canceled. Similarly, errors from the desired effect are preferentially propagated over the genericTimeoutException.The
timeoutAndForgetmethods are left unchanged, as they explicitly avoid waiting for the losing effect to finish.This change allows for
timeoutandtimeoutTomethods to be safely used on effects that acquire resources, such asSemaphore.acquire, ensuring that successful outcomes are always propagated back to the user.