Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace run_forever with start/stop #152

Open
callumforrester opened this issue Jul 11, 2023 · 1 comment
Open

Replace run_forever with start/stop #152

callumforrester opened this issue Jul 11, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@callumforrester
Copy link
Contributor

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 :

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 #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.Events 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

async def run_forever(self) -> None:
    ...

to

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.

@pytest_asyncio.fixture
async def thing() -> Thing:
    thing = Thing(...)
    await thing.start()
    yield thing
    await thing.stop()

Tagging for discussion: @abbiemery @coretl @tpoliaw @DiamondJoseph

@callumforrester callumforrester added the enhancement New feature or request label Jul 11, 2023
@rosesyrett
Copy link
Contributor

@abbiemery has anyone been asigned to do this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants