Skip to content

fix(stream): close the file handle when save_as fails - #3203

Open
Mohammed Alkindi (MohammedAlkindi) wants to merge 1 commit into
microsoft:mainfrom
MohammedAlkindi:fix/stream-close-on-error
Open

Mohammed Alkindi (MohammedAlkindi) wants to merge 1 commit into
microsoft:mainfrom
MohammedAlkindi:fix/stream-close-on-error

Conversation

@MohammedAlkindi

Copy link
Copy Markdown

Stream.save_as closes the file only after the read loop finishes, so an exception from self._channel.send("read", ...) skips the close. Connection.cleanup() raising TargetClosedError into a pending callback is the ordinary way that happens.

The sibling handles the upload direction properly: WritableStream.copy wraps its loop in with open(path, "rb") as f:.

The impact is narrower than it first looks, so to be precise: on CPython an uncaught exception drops the frame and the handle closes anyway. The difference shows when the caller keeps the exception, since __traceback__ retains the frame and its locals:

current       file.closed == False
try/finally   file.closed == True

That is the realistic shape — except Error as e: holding e keeps the frame — and on Windows an open handle also blocks removing or reopening the partial file.

black --check, flake8 and isort --check pass on the changed file, at the versions pinned in .pre-commit-config.yaml.

Could not verify: I did not run the repo's pytest suite; it needs the Node driver, which this machine cannot fetch. The measurement above reproduces the same control flow standalone.

save_as closed the file only after the read loop completed, so an exception from the channel skipped the close. WritableStream.copy handles the upload direction with a context manager already.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant