fix(ImageInput): guarantee close()-on-failure for all open() methods - #5398
Merged
Conversation
brechtvl
reviewed
Aug 14, 2026
brechtvl
left a comment
Contributor
There was a problem hiding this comment.
The changes look good to me, but they appear to be incomplete.
Here's a patch made with Claude assistance to find more missing cases. It includes some drive-by fixes and simplifications that seemed relevant to include.
missed_close_and_other_fixes.patch
If ImageInput::open() encounters an error and returns false, is the object expected to be left in the same state as a newly constructed, never-opened instance (other than having an error that can be retrieved), as if close() had been called? Or is it up to the caller to do the close()? We were never quite clear on this, and some plugins have any open() failures do a close() before returning, but others did not. Clean this up once and for all by documenting ImageInput::open() to make clear that the contract is that a failed open() will close the file and needs no further cleanup action on the part of the caller. Audit all readers and ensure that in every instance that an open() method returns false to indicate an error, close() is also called. Note that: - The error message is stil retrievable from the closed ImageInput. - If an IOProxy was passed to the ImageInput to use, that proxy is still open and able to be used for further reads to the same file. Assisted-by: Claude Code / Claude Sonnet 5 Signed-off-by: Larry Gritz <lg@larrygritz.com> Co-authored by: Brecht Van Lommel <brecht@blender.org>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
lgritz
force-pushed
the
lg-openfailclose
branch
from
August 15, 2026 00:04
8bf751b to
0548339
Compare
Collaborator
Author
|
Amended with your changes, thanks @brechtvl ! |
brechtvl
approved these changes
Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If ImageInput::open() encounters an error and returns false, is the object expected to be left in the same state as a newly constructed, never-opened instance (other than having an error that can be retrieved), as if close() had been called? Or is it up to the caller to do the close()? We were never quite clear on this, and some plugins have any open() failures do a close() before returning, but others did not.
Clean this up once and for all by documenting ImageInput::open() to make clear that the contract is that a failed open() will close the file and needs no further cleanup action on the part of the caller.
Audit all readers and ensure that in every instance that an open() method returns false to indicate an error, close() is also called.
Note that:
Assisted-by: Claude Code / Claude Sonnet 5
Co-authored by: Brecht Van Lommel brecht@blender.org