context manager for BlockTools #20326
Conversation
emlowe
added
CI
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| create_block_tools_async( | ||
| constants=consensus_constants, keychain=keychain2, config_overrides=config_overrides | ||
| ) | ||
| ) |
There was a problem hiding this comment.
it's not obvious that it's safe to start these block tools after we try to find available local listen ports. They may try to open listen sockets themselves. I think it would be safer to move these up to create them in the original order
There was a problem hiding this comment.
Hey, bugbot found these issues as well
chia/_tests/conftest.py
Outdated
|
|
||
| @pytest.fixture(scope="session", name="bt") | ||
| async def block_tools_fixture(get_keychain, blockchain_constants, anyio_backend, testrun_uid: str) -> BlockTools: | ||
| async def block_tools_fixture(get_keychain, blockchain_constants, anyio_backend, testrun_uid: str): |
There was a problem hiding this comment.
ideally we would keep the type annotation for the return value
| ) | ||
|
|
||
| def __enter__(self) -> Self: | ||
| return self |
There was a problem hiding this comment.
wait, aren't you making this an async context manager? Then you need __aenter__() and __aexit__(), don't you?
There was a problem hiding this comment.
The BlockTools class is always used with as the class itself doesn't need any async functionality to get an instance. create_block_tools_async is an asynccontextmanager and I think it's ok for it to use BlockTools with and not async with
Quexington
left a comment
Quexington
left a comment
There was a problem hiding this comment.
Pending other reviewers' comments of course but I didn't find anything else to comment on.
|
Add context feature to
BlockToolsand makecreate_block_tools_asynca context managerThis allows for the automatic cleanup of the TemporaryDirectory that BlockTools may create during initialization. Python 3.14 has more checks for implicit cleanup of temp dirs and this required an ignore being added to pytest.ini otherwise tests were being marked as failed due to ResourceWarnings
This code only affects tests, and although the specific problem can be worked around in
pytest.ini, this is a "more correct" solution as it explicitly calls the directory cleanup function.Note
Convert
BlockToolsandcreate_block_tools(_async)into (async) context managers and update tests/benchmarks/tools to use them, improving resource cleanup.BlockTools(__enter__/__exit__) with explicit tempdir cleanup.create_block_tools_asyncto an@asynccontextmanageryieldingBlockTools.create_block_toolsto a@contextmanageryieldingBlockTools.bt,get_b_tools*, custom tools) to returnAsyncIterator[BlockTools]and useasync with create_block_tools_async(...).async withacross blockchain, full node, farmer/harvester, solver, simulation, and setup helpers.benchmarks/blockchains.py) andtools/generate_chain.pyto use context-managedBlockTools/keyrings.Written by Cursor Bugbot for commit 5565471. This will update automatically on new commits. Configure here.