DM-36207: Reset the Fits object status to 0 if we are throwing an error #657
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
timj
approved these changes
Sep 13, 2022
|
FYI @timj I'm pretty sure for some reason I don't have permission to run it in Jenkins, not sure what the glitch is there... |
|
Well, we have to figure out the jenkins thing. You should also change the title of this PR to |
weatherhead99
changed the title
|
On Slack you considered multiple approaches to fixing this. Could I encourage you to capture the logic from those considerations in the commit message so it's not (effectively) lost? |
The nature of the cfitsio library is that many calls fail when the status value passed in is not set to a value indicating "success". Prior to this patch we did not reset that status value after throwing an error, and since the status value is part of the state of the Fits class, any operation after an exception was thrown was liable (though not guaranteed) to fail purely because some previous (invalid) operation failed. Most notably, when moving to an invalid HDU and back again, moving back would fail for this reason. Another possible solution to this would be to manually reset status every time we make a cfitsio call, or to alter the LSST_FITS_EXCEPT macro to take a status allowing us to store a temporary and reset the class held value before throwing. The solution chosen, was to construct the exception as per previously, then store it, reset the class status and finally throw the exception.
weatherhead99
force-pushed
the
tickets/DM-36207
branch
from
fc4f519
to
190dffa
Compare
|
ok I believe all comments have been accounted for and I have squashed this to one commit with a hopefully informative message. Only thing remaining is the Jenkins run I think |
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.
Fix and test case added for https://jira.lsstcorp.org/browse/DM-36207.