free tree on AsdfFile.close #1575
Merged
Conversation
This file contains 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
WilliamJamieson
previously requested changes
Jul 3, 2023
eslavich
reviewed
Jul 31, 2023
| @@ -449,6 +449,9 @@ def close(self): | |||
| self._fd.close() | |||
| self._fd = None | |||
| self._closed = True | |||
| # as we're closing the file, also empty out the | |||
| # tree so that references to array data can be released | |||
| self._tree = AsdfObject() | |||
There was a problem hiding this comment.
What do you think about raising a relevant error when the tree of a closed AsdfFile is accessed? I'd prefer a message like "Sorry your AsdfFile is closed" to "KeyError" if I were investigating a bug in my code.
There was a problem hiding this comment.
A custom exception sounds like a great idea. Currently and OSError is raised so any custom exception we raise could inherit from that exception class for backwards compatibility.
Lines 559 to 561 in b43b2c5
It might make sense to raise the same error type when a block is accessed from a closed file (and any other places where access to a closed file is prevented):
Lines 1290 to 1292 in b43b2c5
Perhaps a
ClosedFileAccess exception that inherits from OSerror? That seems reasonably backwards compatible to include in 3.0 (and would work towards cleaning up the rather generic exceptions raised by asdf).
There was a problem hiding this comment.
Oh, oops, I didn't notice/remember that we already throw an exception. OSError is plenty enough for me.
With these changes, when an AsdfFile object is closed the tree (which already could not be accessed because of the tree property gatekeeping) is set to a blank AsdfObject. This allows the garbage collector to clean up objects referenced in the tree even though the AsdfFile object does not fall out of scope. Fixes asdf-format#1558
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.
With these changes, when an AsdfFile object is closed the tree (which already could not be accessed because of the tree property gatekeeping) is set to a blank AsdfObject.
This allows the garbage collector to clean up objects referenced in the tree even though the AsdfFile object does not fall out of scope.
Fixes #1558