`fail_after` deadline is set on initialization not context entry

[zanieb]

This is a duplicate of agronholm/anyio#514 — they are following the convention established here in trio.

When using a fail_after context, the deadline is set at "initialization" time rather than __enter__. This behavior can result in unexpected bugs if the context is declared before it is entered. This is not particularly intuitive for a Python context manager — I'd expect the timer to start when the context is entered. I do not think changing it would be complex, but it could be considered breaking.

import trio


async def main():
    ctx = trio.fail_after(5)
    await trio.sleep(5)
    with ctx:
        for i in range(1, 6):
            print(i)
            await trio.sleep(1)


trio.run(main)

❯ python example.py  
1
Traceback (most recent call last):
  File "/opt/homebrew/Caskroom/miniconda/base/envs/orion-dev-39/lib/python3.9/site-packages/trio/_timeouts.py", line 106, in fail_at
    yield scope
  File "/Users/mz/dev/prefect/example.py", line 168, in main
    await trio.sleep(1)
  File "/opt/homebrew/Caskroom/miniconda/base/envs/orion-dev-39/lib/python3.9/site-packages/trio/_timeouts.py", line 76, in sleep
    await sleep_until(trio.current_time() + seconds)
  File "/opt/homebrew/Caskroom/miniconda/base/envs/orion-dev-39/lib/python3.9/site-packages/trio/_timeouts.py", line 57, in sleep_until
    await sleep_forever()
  File "/opt/homebrew/Caskroom/miniconda/base/envs/orion-dev-39/lib/python3.9/site-packages/trio/_timeouts.py", line 40, in sleep_forever
    await trio.lowlevel.wait_task_rescheduled(lambda _: trio.lowlevel.Abort.SUCCEEDED)
  File "/opt/homebrew/Caskroom/miniconda/base/envs/orion-dev-39/lib/python3.9/site-packages/trio/_core/_traps.py", line 166, in wait_task_rescheduled
    return (await _async_yield(WaitTaskRescheduled(abort_func))).unwrap()
  File "/opt/homebrew/Caskroom/miniconda/base/envs/orion-dev-39/lib/python3.9/site-packages/outcome/_impl.py", line 138, in unwrap
    raise captured_error
  File "/opt/homebrew/Caskroom/miniconda/base/envs/orion-dev-39/lib/python3.9/site-packages/trio/_core/_run.py", line 1222, in raise_cancel
    raise Cancelled._create()
trio.Cancelled: Cancelled

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/mz/dev/prefect/example.py", line 171, in <module>
    trio.run(main)
  File "/opt/homebrew/Caskroom/miniconda/base/envs/orion-dev-39/lib/python3.9/site-packages/trio/_core/_run.py", line 2010, in run
    raise runner.main_task_outcome.error
  File "/Users/mz/dev/prefect/example.py", line 168, in main
    await trio.sleep(1)
  File "/opt/homebrew/Caskroom/miniconda/base/envs/orion-dev-39/lib/python3.9/contextlib.py", line 137, in __exit__
    self.gen.throw(typ, value, traceback)
  File "/opt/homebrew/Caskroom/miniconda/base/envs/orion-dev-39/lib/python3.9/site-packages/trio/_timeouts.py", line 108, in fail_at
    raise TooSlowError
trio.TooSlowError

[Fuyukai]

So don't do that? I fail to see any good design where creating a context manager and then not using it would make sense.

[zanieb]

Often, we'll create a context manager ahead of time (during a configuration step, for example) then downstream enter the context. In my application, we are using fail_after to enforce configurable timeouts. We set up the context but do not want to enter it until right before we call the (library user's) code that needs a timeout. At other times, when there is not a timeout, we'll use a null context to simplify downstream logic.

If we weren't ever using the context manager, this wouldn't be an issue. The issue here is that the default behavior is antithetical to typical context manager behavior. It's like having the following return the amount of time since initialization rather than within the context:

timer = timer()
with timer:
   ...

It's also not an atypical design to use things like an AsyncExitStack — in this case developers may create the contexts beforehand then enter them in a loop.

[agronholm]

Trio would have to change their semantics from what has been documented, and so far there haven't been any practical use cases that would warrant this.

[zanieb]

@agronholm can you link to the documentation that covers this behavior? I did not see any explicit mention of this.

[agronholm]

Here: https://trio.readthedocs.io/en/stable/reference-core.html#trio.fail_after

Creates a cancel scope with the given timeout, and raises an error if it is actually cancelled.

Implies that the cancel scope gets created with a set deadline. Says nothing about the deadline getting set when entering the scope.

[zanieb]

Ah I think that can be interpreted either way based on your assumptions.. the example shows with fail_after() as cancel_scope so it's pretty ambiguous when that scope is created — it could just as well be created on __enter__ and yielded. In fact, you cannot get the CancelScope object itself until after entering the context (in anyio).

[agronholm]

As there are 0 examples of separating the instantiation of this context manager from its use in a with statement in the documentation of either trio or anyio, I think it would be safe to assume that it was not a supported use case.

[zanieb]

If it was not a supported use-case then what is the objection to changing the behavior to be clearer?

[zanieb]

To be clear: I can work around this and will need to do so for compatibility with older versions of anyio/trio no matter what is decided here. I will likely add my own utility with the behavior I need — I am only advocating for an API that is less likely to be misleading here.

[agronholm]

If it was not a supported use-case then what is the objection to changing the behavior to be clearer?

If anybody anywhere is already using the context manager in such a disjointed manner correctly, their code will be broken. I wouldn't expect anybody to be doing that, but then again they wouldn't report that to us unless something wasn't working. I also didn't expect anybody to be using the cm like this at all, so who knows?

[agronholm]

I think a change in semantics should be considered but only if someone brings forth a plausible use case where doing this is necessary or at least beneficial.

[zanieb]

Yeah unfortunately timeouts are kind of fuzzy in the first place — we had it like this for a long time before we got a bug report from a user. From a cursory search on GitHub, I don't see any uses apart from ours where the initialization and entry are separated. I'll open a pull request to document this caveat. We can see if more people chime in on this issue, I see no reason to rush an API change.

[njsmith]

I don't think there's any problem necessarily with calculating the deadline at the last moment, though I can imagine someone being confused the other way as well. ("I created the fail_after object early because I wanted that to be when the deadline was relative to.")

BTW, re:

In my application, we are using fail_after to enforce configurable timeouts. We set up the context but do not want to enter it until right before we call the (library user's) code that needs a timeout. At other times, when there is not a timeout, we'll use a null context to simplify downstream logic.

Trio accepts math.inf as a timeout everywhere, meaning "never timeout". So you could pass the bare timeout value around everywhere (either the configured timeout, or math.inf if there's no timeout), and then unconditionally call fail_after around the operation that has the timeout. Might be easier to understand, independently of whatever happens with this discussion?

[smurfix]

I'd vote +0.25 for changing the behavior (but +1 for documenting things more stringently).

("I created the fail_after object early because I wanted that to be when the deadline was relative to."

You can easily calculate an absolute timeout at that time instead.

The way things currently are, you can't achieve the behavior @madkinsz wants as easily, thus this'd be a net win for that kind of flexibility in case anybody ever wants it.