Shield file stream cleanup from cancellation - #1318
Conversation
agronholm
left a comment
There was a problem hiding this comment.
The added tests seem to also try to validate the cancellation mechanism. There are other tests to cover those, so these parts are just unnecessary noise here, agreed?
|
|
||
| assert scope.cancelled_caught | ||
| assert file.closed | ||
| assert file_path.read_bytes() == b"Hello" |
There was a problem hiding this comment.
Is there a situation where the test would fail at this point?
There was a problem hiding this comment.
Agreed: once this real buffered file is closed, the read-back assertion only repeats ordinary write/close behavior covered elsewhere. In 261034c I removed that assertion and its payload write, along with the explicit checkpoint, pytest.fail() and cancelled_caught checks from both tests. Each regression now only triggers cancellation during context exit and asserts that the real file is closed.
The simplified tests still fail in all eight cases on the original implementation (asyncio, winloop, eager asyncio and Trio); the 131 related file-stream/AsyncFile/stapled-stream tests pass. The production fix is unchanged.
| with CancelScope(shield=True): | ||
| await to_thread.run_sync(self._file.close) |
There was a problem hiding this comment.
I think this should be like trio and include a checkpoint_if_cancelled:
There was a problem hiding this comment.
Added in 4d8114e: the shielded file close completes first, then checkpoint_if_cancelled() delivers pending cancellation, matching the linked implementation.
The two existing regressions now expect cancellation from context exit itself and check that the real file was closed. They fail in all eight backend cases on the previous shield-only commit (no cancellation raised), and also on the original implementation (file left open). The 131 related file-stream/AsyncFile/stapled-stream tests and focused mypy pass; non-mypy pre-commit hooks pass.
NOTE Erasing or replacing the contents of this template will result in your pull
request being summarily closed without consideration!
Changes
Related to #1314.
When a cancelled task exits a
FileReadStreamorFileWriteStreamcontext,to_thread.run_sync()can raise before dispatchingfile.close(), leaving thefile open. Shield the shared close operation, then call
checkpoint_if_cancelled()after the file is closed so cancellation is deliveredby the close operation itself.
The regressions use public
from_path()contexts and assert that closing bothreleases the real file and propagates cancellation. They have no independent
checkpoint, scope-state or ordinary write/read-back assertions.
Validation on Python 3.13.14 / Windows:
and on the shield-only version because close does not raise cancellation.
Each comparison has 8 failures across asyncio, winloop, eager asyncio and Trio.
earlier wider file/path run had 372 passes, 96 skips and 76 Windows symlink
failures/setup errors; all 76 also reproduced on the unchanged base. These
platform-dependent checks are not claimed as passing.
The changelog records the fix; no user-guide addition is needed for expected
cleanup behavior.
Checklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
You've added tests (in
tests/) which would fail without your patchYou've updated the documentation (in
docs/), in case of behavior changes or newfeatures
docs/versionhistory.rst).If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.
Updating the changelog
If there are no entries after the last release, use
**UNRELEASED**as the version.If, say, your patch fixes issue #123, the entry should look like this:
If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.