Open biochimia opened 2 months ago
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.
I'm not sure if the tests should go elsewhere.
You could add them here as well.
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/
.
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 undertests/
?
If it is not testing something specific to IO
or the runtime, then I prefer to test it with PureConc
.
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:
onCancel
to observe the cancellation of a timed out action, but the timeout gets triggered without the finaliser being invoked.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.
- 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 😳
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?
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, aTimeoutException
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
andtimeoutTo
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 aTimeoutException
if 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
timeoutAndForget
methods are left unchanged, as they explicitly avoid waiting for the losing effect to finish.This change allows for
timeout
andtimeoutTo
methods to be safely used on effects that acquire resources, such asSemaphore.acquire
, ensuring that successful outcomes are always propagated back to the user.