DiamondLightSource / DiamondLightSource/tickit
Replace run_forever with start/stop
- Dominant language
- Python
- Stars
- 7
- Forks
- 1
- PR merge metrics
- No merged PRs in 30d
Description
**Speculative Issue for Discussion**
The `run_forever()` API is prevalent throughout tickit. Components, adapters, schedulers etc. all have it. The idea is to collect this bucket of things into a giant asyncio task that is essentially :
```python
asyncio.wait([thing.run_forever() for thing in all things])
```
This is convenient for running arbitrary simulations but painful to test. Tests need to know a component/adapter/scheduler has completed startup tasks and is ready to accept API calls. See https://github.com/dls-controls/tickit/issues/76. `run_forever()` often performs startup tasks opaquely, so the tests can't know when it's done. There have been various workarounds throughout the codebase such as exposing `asyncio.Event`s or just having a sleep to allow servers time to start. None of these are ideal.
We also cannot easily shut things down between tests, there have been instances of tests breaking other tests. In some cases we have restored forcing task cancels.
I propose we go through and change all of these APIs from
```python
async def run_forever(self) -> None:
...
```
to
```python
async def start(self) -> None:
...
async def stop(self) -> None:
...
```
The contract then is that `start()` performs all startup tasks and can be awaited on and `stop()` can be awaited on until all tasks have shut down. It is then easy to offer a fixture for a particular component et al.
```python
@pytest_asyncio.fixture
async def thing() -> Thing:
thing = Thing(...)
await thing.start()
yield thing
await thing.stop()
```
Tagging for discussion: @abbiemery @coretl @tpoliaw @DiamondJoseph
Contributor guide
Assessment
This issue has not been assessed yet.