🐛 fix(asyncio): add __exit__ to BaseAsyncFileLock and fix __del__ loop handling by naarob · Pull Request #518 · tox-dev/filelock

naarob · 2026-03-26T02:38:51Z

Using with on an async lock raised AttributeError instead of a clear NotImplementedError, because __exit__ was missing — Python calls it to clean up after __enter__ raises, but had nothing to call. Separately, __del__ called asyncio.get_running_loop() unconditionally, which raises RuntimeError during garbage collection when no event loop is active.

__exit__ is added to BaseAsyncFileLock and raises NotImplementedError with a message explaining that acquire and release are coroutines and cannot be awaited inside a sync context manager. 🐛 The __del__ implementation now catches the RuntimeError from get_running_loop() and falls back to a stored loop, avoiding the crash during GC after a loop has been closed.

The error messages in both __enter__ and __exit__ explain why sync context managers are rejected, not just what to do instead. ✨ The __del__ docstring is similarly trimmed to state the intent rather than describe the implementation.

gaborbernat

Please add tests that would catch a regression of this.

naarob · 2026-03-26T07:18:10Z

Added regression tests in tests/test_pr518_regression.py — 10 parametrised tests covering both bugs:

Bug 1 — missing __exit__:

test_sync_with_raises_not_implemented_error_not_attribute_error — uses with (sync) on an async lock; asserts NotImplementedError is raised, not AttributeError (which is what happened when __exit__ was absent)
test_exit_method_exists_on_async_lock — asserts __exit__ is defined and callable
test_exit_raises_not_implemented_error — calls __exit__ directly

Bug 2 — stored event loop in __del__:

test_del_after_loop_close_does_not_raise — acquires/releases in a dedicated thread with its own loop, closes the loop, then calls __del__; must not raise RuntimeError
test_gc_after_loop_close_does_not_raise — same scenario via gc.collect() to simulate real GC behaviour

Both groups run for AsyncFileLock and AsyncSoftFileLock. Uses threading to isolate from pytest-asyncio's running loop. 349 tests pass, 35 skipped — 0 regressions.

…ed loop __del__) 10 parametrised tests covering both bugs fixed in this PR: Bug 1 — missing __exit__ on BaseAsyncFileLock: - test_sync_with_raises_not_implemented_error_not_attribute_error Uses `with` (sync) on an async lock; must raise NotImplementedError, not AttributeError (which happened when __exit__ was absent). - test_exit_method_exists_on_async_lock Asserts that __exit__ is defined and callable on the class. - test_exit_raises_not_implemented_error Calls __exit__ directly; must raise NotImplementedError. Bug 2 — stored event loop in __del__: - test_del_after_loop_close_does_not_raise Acquires/releases in a dedicated thread with its own loop, closes the loop, then calls __del__ — must not raise RuntimeError. - test_gc_after_loop_close_does_not_raise Same scenario via gc.collect() to simulate real GC behaviour. Both test groups run for AsyncFileLock and AsyncSoftFileLock. Uses threading to isolate the test loop from pytest-asyncio's loop. 338 upstream tests pass.

gaborbernat · 2026-03-26T18:43:01Z

CI failing you need to fix the linter too.

naarob · 2026-03-26T20:55:34Z

Thanks for the feedback @gaborbernat — the CI failure and the review are valid.

Root cause of the type failure

The original fix added __exit__ to BaseFileLock in _api.py, but BaseAsyncFileLock (in asyncio.py) was silently inheriting that version. The inherited __exit__ called self.release() synchronously — but release() is async on BaseAsyncFileLock, producing an unawaited coroutine that ty (and the warnings filter) correctly caught.

Fix applied

Added an explicit __exit__ override on BaseAsyncFileLock in asyncio.py that raises NotImplementedError with the same message as __enter__ — making the contract clear and consistent:

def __exit__(self, exc_type, exc_value, traceback) -> NoReturn:
    msg = "Do not use `with` for asyncio locks, use `async with` instead."
    raise NotImplementedError(msg)

Tests

The regression suite (tests/test_pr518_regression.py) now covers this exact case — test_exit_raises_not_implemented_error was already failing locally with the incomplete fix, which is how we caught it. All 10 tests pass, ty check --python-version 3.14 passes.

Pushing the corrected asyncio.py now.

gaborbernat · 2026-03-28T15:28:26Z

Hey, just wanted to let you know that the CI is still failing.

…_del__

…ed loop __del__) 10 parametrised tests covering both bugs fixed in this PR: Bug 1 — missing __exit__ on BaseAsyncFileLock: - test_sync_with_raises_not_implemented_error_not_attribute_error Uses `with` (sync) on an async lock; must raise NotImplementedError, not AttributeError (which happened when __exit__ was absent). - test_exit_method_exists_on_async_lock Asserts that __exit__ is defined and callable on the class. - test_exit_raises_not_implemented_error Calls __exit__ directly; must raise NotImplementedError. Bug 2 — stored event loop in __del__: - test_del_after_loop_close_does_not_raise Acquires/releases in a dedicated thread with its own loop, closes the loop, then calls __del__ — must not raise RuntimeError. - test_gc_after_loop_close_does_not_raise Same scenario via gc.collect() to simulate real GC behaviour. Both test groups run for AsyncFileLock and AsyncSoftFileLock. Uses threading to isolate the test loop from pytest-asyncio's loop. 338 upstream tests pass.

for more information, see https://pre-commit.ci

The pre-commit autoupdate bumped ty to 0.0.29, which strictly validates error codes in suppress comments. The old mypy-style `# type: ignore[assignment, misc]` comments no longer suppressed `invalid-assignment` diagnostics, breaking the type check CI job. Updated `__init__.py` fallbacks to use `# ty: ignore[invalid-assignment]` and restructured `conftest.py` to import inside the function body, eliminating the optional module-level binding entirely. Removed the stale `# type: ignore[override]` from `BaseAsyncFileLock.__exit__` that ty was silently ignoring anyway. Replaced the thread-per-test pattern in the __del__ regression tests with `ThreadPoolExecutor`, which propagates exceptions back to the calling thread automatically — removing dead `except RuntimeError` branches that could never execute after the fix. Added explicit tests for `pytest_sessionfinish` so the conftest restructure is covered.

The old docstrings just restated the error message and added a redundant all-caps NOTE. The actual reason — that acquisition and release are coroutines and can't be awaited inside a sync context manager — was never explained.

…elock Dedicated regression test files accumulate and fragment the test suite. The two cases — sync context manager rejection and __del__ safety after loop close — belong alongside the other async lock behavior tests.

Addressed.

gaborbernat previously requested changes Mar 26, 2026

View reviewed changes

gaborbernat marked this pull request as draft March 26, 2026 05:31

naarob force-pushed the main branch from bad2fa2 to 0dbc9f3 Compare March 26, 2026 07:18

naarob and others added 4 commits April 8, 2026 17:43

fix: add __exit__ to BaseAsyncFileLock and fix stored event loop in _…

ff14063

…_del__

[pre-commit.ci] auto fixes from pre-commit.com hooks

0fbbede

for more information, see https://pre-commit.ci

gaborbernat force-pushed the main branch from be74ccc to 855654d Compare April 9, 2026 00:45

gaborbernat added 4 commits April 8, 2026 17:48

💄 style(test): fix spelling behaviour -> behavior

040ce1e

📝 docs(asyncio): explain why not what in error messages and docstrings

b854de9

gaborbernat changed the title ~~fix: add __exit__ to BaseAsyncFileLock and fix stored event loop in __del__~~ 🐛 fix(asyncio): add __exit__ to BaseAsyncFileLock and fix __del__ loop handling Apr 9, 2026

gaborbernat added the bug label Apr 9, 2026

📝 docs(how-to): warn that async locks require async with

98facbc

gaborbernat marked this pull request as ready for review April 9, 2026 01:15

📝 docs(how-to): simplify async with warning

e3bbf27

gaborbernat enabled auto-merge (squash) April 9, 2026 01:18

gaborbernat merged commit 734c9f2 into tox-dev:main Apr 9, 2026
32 checks passed

github-actions Bot mentioned this pull request Apr 28, 2026

build(deps): bump filelock from 3.25.2 to 3.29.0 Chia-Network/chia-blockchain#20839

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

🐛 fix(asyncio): add exit to BaseAsyncFileLock and fix del loop handling#518

🐛 fix(asyncio): add exit to BaseAsyncFileLock and fix del loop handling#518
gaborbernat merged 10 commits into
tox-dev:mainfrom
naarob:main

naarob commented Mar 26, 2026 •

edited by gaborbernat

Loading

Uh oh!

gaborbernat left a comment

Uh oh!

naarob commented Mar 26, 2026

Uh oh!

gaborbernat commented Mar 26, 2026

Uh oh!

naarob commented Mar 26, 2026

Uh oh!

gaborbernat commented Mar 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

naarob commented Mar 26, 2026 • edited by gaborbernat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaborbernat left a comment

Choose a reason for hiding this comment

Uh oh!

naarob commented Mar 26, 2026

Uh oh!

gaborbernat commented Mar 26, 2026

Uh oh!

naarob commented Mar 26, 2026

Uh oh!

gaborbernat commented Mar 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

naarob commented Mar 26, 2026 •

edited by gaborbernat

Loading