sim: call .aclose() on TickTrigger in .until() and .repeat()

[whitequark]

Otherwise, when used together with asyncio, the following warning will be printed for each use of .until()/.repeat():

E: asyncio: Task was destroyed but it is pending!
task: <Task pending name='Task-2' coro=<<async_generator_athrow without __name__>()>>
/usr/lib/python3.13/asyncio/base_events.py:744: RuntimeWarning: coroutine method 'aclose' of 'TickTrigger.__aiter__' was never awaited

[mabl]

I am unfortunately running into random failures with this change in my testcases:

  + Exception Group Traceback (most recent call last):
  |   File "C:\path\.venv\Lib\site-packages\_pytest\runner.py", line 344, in from_call
  |     result: TResult | None = func()
  |                              ^^^^^^
  |   File "C:\path\.venv\Lib\site-packages\_pytest\runner.py", line 246, in <lambda>
  |     lambda: runtest_hook(item=item, **kwds), when=when, reraise=reraise
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |   File "C:\path\.venv\Lib\site-packages\pluggy\_hooks.py", line 513, in __call__
  |     return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |   File "C:\path\.venv\Lib\site-packages\pluggy\_manager.py", line 120, in _hookexec
  |     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |   File "C:\path\.venv\Lib\site-packages\pluggy\_callers.py", line 139, in _multicall
  |     raise exception.with_traceback(exception.__traceback__)
  |   File "C:\path\.venv\Lib\site-packages\pluggy\_callers.py", line 122, in _multicall
  |     teardown.throw(exception)  # type: ignore[union-attr]
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^
  |   File "C:\path\.venv\Lib\site-packages\_pytest\logging.py", line 850, in pytest_runtest_call
  |     yield
  |   File "C:\path\.venv\Lib\site-packages\pluggy\_callers.py", line 122, in _multicall
  |     teardown.throw(exception)  # type: ignore[union-attr]
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^
  |   File "C:\path\.venv\Lib\site-packages\_pytest\capture.py", line 900, in pytest_runtest_call
  |     return (yield)
  |             ^^^^^
  |   File "C:\path\.venv\Lib\site-packages\pluggy\_callers.py", line 122, in _multicall
  |     teardown.throw(exception)  # type: ignore[union-attr]
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^
  |   File "C:\path\.venv\Lib\site-packages\_pytest\skipping.py", line 263, in pytest_runtest_call
  |     return (yield)
  |             ^^^^^
  |   File "C:\path\.venv\Lib\site-packages\pluggy\_callers.py", line 103, in _multicall
  |     res = hook_impl.function(*args)
  |           ^^^^^^^^^^^^^^^^^^^^^^^^^
  |   File "C:\path\.venv\Lib\site-packages\_pytest\unraisableexception.py", line 158, in pytest_runtest_call
  |     collect_unraisable(item.config)
  |   File "C:\path\.venv\Lib\site-packages\_pytest\unraisableexception.py", line 81, in collect_unraisable
  |     raise ExceptionGroup("multiple unraisable exception warnings", errors)
  | ExceptionGroup: multiple unraisable exception warnings (2 sub-exceptions)
  +-+---------------- 1 ----------------
    | Traceback (most recent call last):
    |   File "C:\path\.venv\Lib\site-packages\amaranth\sim\_async.py", line 358, in until
    |     clk, rst, *values, done = await tick.__anext__()
    |                               ^^^^^^^^^^^^^^^^^^^^^^
    | GeneratorExit
    |
    | During handling of the above exception, another exception occurred:
    |
    | Traceback (most recent call last):
    |   File "C:\path\tests\protocol\test_parse_recordings.py", line 78, in testbench_output_no_error
    |     await ctx.tick().until(dut.error_state == 1)
    |   File "C:\path\.venv\Lib\site-packages\amaranth\sim\_async.py", line 363, in until
    |     await tick.aclose()
    | RuntimeError: aclose(): asynchronous generator is already running
    |
    | The above exception was the direct cause of the following exception:
    |
    | Traceback (most recent call last):
    |   File "C:\path\.venv\Lib\site-packages\_pytest\unraisableexception.py", line 67, in collect_unraisable
    |     warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
    | pytest.PytestUnraisableExceptionWarning: Exception ignored in: <coroutine object TestParseRecordings.test_parse_recordings.<locals>.testbench_output_no_error at 0x000002C4FE47B8A0>
    | Enable tracemalloc to get traceback where the object was allocated.
    | See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.
    +---------------- 2 ----------------
    | Traceback (most recent call last):
    |   File "C:\path\.venv\Lib\site-packages\amaranth\sim\_async.py", line 358, in until
    |     clk, rst, *values, done = await tick.__anext__()
    |                               ^^^^^^^^^^^^^^^^^^^^^^
    | GeneratorExit
    |
    | During handling of the above exception, another exception occurred:
    |
    | Traceback (most recent call last):
    |   File "C:\path\tests\protocol\test_parse_recordings.py", line 68, in testbench_output
    |     await stream_get(ctx, dut.o_stream)
    |   File "C:\path\tests\helpers\stream.py", line 8, in stream_get
    |     payload, = await ctx.tick().sample(s.payload).until(s.valid)
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |   File "C:\path\.venv\Lib\site-packages\amaranth\sim\_async.py", line 363, in until
    |     await tick.aclose()
    | RuntimeError: aclose(): asynchronous generator is already running
    |
    | The above exception was the direct cause of the following exception:
    |
    | Traceback (most recent call last):
    |   File "C:\path\.venv\Lib\site-packages\_pytest\unraisableexception.py", line 67, in collect_unraisable
    |     warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
    | pytest.PytestUnraisableExceptionWarning: Exception ignored in: <coroutine object TestParseRecordings.test_parse_recordings.<locals>.testbench_output at 0x000002C4989AEB20>
    | Enable tracemalloc to get traceback where the object was allocated.
    | See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.
    +------------------------------------

I'm wondering, why the code needs to be that complex? Can't we simply do

        async for clk, rst, *values, done in self.sample(condition):
            if rst:
                raise DomainReset
            if done:
                return tuple(values)

That also fixes the problem for me.

The other code segment would probably be written as

        count = operator.index(count)
        if count <= 0:
            raise ValueError(f"Repeat count must be a positive integer, not {count!r}")

        result = None
        async for i, (clk, rst, *values) in enumerate(self):
            if i >= count:
                break
            if rst:
                raise DomainReset
            assert clk
            result = tuple(values)
        return result

but I have not tested that.

[whitequark]

Yes, I've seen these failures too.

[whitequark]

Would you like to submit your changes as a PR?

[mabl]

Yes, I've seen these failures too.

Alright, glad I don't need to build a simplified test case to generate it.

Would you like to submit your changes as a PR?

Thinking about it some more, I am not sure if my proposed fix actually calls aclose() and hence adresses the original goal of this MR...

I tried

        async with contextlib.aclosing(self.sample(condition).__aiter__()) as tick:
            async for clk, rst, *values, done in tick:
                if rst:
                    raise DomainReset
                if done:
                    return tuple(values)

but ran into the same issues that the manual handlin in master is having...

[whitequark]

Truth be told, I don't actually understand the semantics of / need for aclose here...

[whitequark]

I don't need to build a simplified test case to generate it.

Not to prove something to myself, no, but it might be quite useful to have one for the purpose of understanding this problem better, and also in order to have a regression test.

I think asyncio collects all of the async generators you await and then calls .aclose() on them later or something? Which is something that the Amaranth simulator does not. There might be more than one bug here.

[agrif]

I think I ran in to this issue today, or one close to it. I'm doing something like this small example:

import amaranth as am
import amaranth.sim

async def deadline_checker(ctx):
    await ctx.tick().repeat(100)
    raise RuntimeError('deadline')

m = am.Module()
counter = am.Signal(8)
m.d.sync += counter.eq(counter + 1)

sim = am.sim.Simulator(m)
sim.add_process(deadline_checker)

sim.run()

My intention is to run a background process that throws an error after some synchronous deadline, but otherwise is dropped if the rest of the simulation ends. Under Python 3.13.5, this works fine. With Python 3.12.8, I get

Exception ignored in: <coroutine object deadline_checker at 0x7f433c8c9f00>
Traceback (most recent call last):
  File "/home/agrif/devel/fpga/simulator_aclose.py", line 7, in deadline_checker
  File "/home/agrif/devel/fpga/local/nara-env-312/lib/python3.12/site-packages/amaranth/sim/_async.py", line 414, in repeat
RuntimeError: aclose(): asynchronous generator is already running

In general, I get this exception in 3.12 every time a repeat(..) is interrupted, although I haven't tested if this happens when it's used as an async iterator.