Skip to content

Error handler revamp#647

Open
jhunkeler wants to merge 18 commits into
spacetelescope:mainfrom
jhunkeler:error-handler-revamp
Open

Error handler revamp#647
jhunkeler wants to merge 18 commits into
spacetelescope:mainfrom
jhunkeler:error-handler-revamp

Conversation

@jhunkeler

@jhunkeler jhunkeler commented May 3, 2025

Copy link
Copy Markdown
Contributor

All instrument packages provide their own error reporting routine (pkg/[inst]/whicherror.c:WhichError) however the code is effectively the same wherever it appears. The only exception is that when status was to set to ACS_OK, WFC3_OK, or STIS_OK (all three are defined as 0) the function returns early.

This PR breaks the error reporting function into two:

  1. hstcal_error_state_populate - responsible for initializing a hstcal_error_state structure
  2. hstcal_error_state_show - responsible for displaying error information, similar to WhichError.

And introduces two macros:

  1. REPORT_ERROR_STATE() - Effectively replaces WhichError, and does not require any arguments
  2. WhichError() - Is a wrapper. The argument normally accepted by the old WhichError is ignored, but allows the existing hstcal to work as-is.

The hstcal_error_state structure just keeps all of HSTCAL's errors in one convenient place.

No additional memory is allocated by these functions, so they can be called from anywhere without fear of accidentally introducing leaks. To keep things clean and simple the backtrace feature writes directly to the stderr descriptor to avoid calling malloc if/when we're already out of RAM.

Example error (with -DENABLE_BACKTRACE=ON):

CALBEG*** ACSCCD -- Version 10.4.0 (07-May-2024) ***
Begin    03-May-2025 18:31:03 EDT
Input    j8cd02tyq_raw.fits
Output   j8cd02tyq_blv_tmp.fits
Warning    Output file `j8cd02tyq_blv_tmp.fits' already exists.
ERROR:    Couldn't process CCD data
ERROR:    CALACS processing NOT completed for j8cd02011_asn.fits
ERROR:    Invalid temporary file. (status = 1021)
DEBUG:    Location: /home/example/hstcal/pkg/acs/src/mainacs.c:238
DEBUG:    Backtrace:
/home/example/hstcal/cmake-build-debug/lib/libhstcalib.so(hstcal_error_state_populate+0xa3) [0x7ddb5c635987]
/home/example/hstcal/cmake-build-debug/lib/libhstcalib.so(hstcal_error_state_show+0x39) [0x7ddb5c635e8d]
/home/example/hstcal/cmake-build-debug/pkg/acs/calacs.e(main+0x990) [0x5760d5ad3c4b]
/usr/lib/libc.so.6(+0x27488) [0x7ddb5c035488]
/usr/lib/libc.so.6(__libc_start_main+0x8c) [0x7ddb5c03554c]
/home/example/hstcal/cmake-build-debug/pkg/acs/calacs.e(_start+0x25) [0x5760d5ad31c5]

Example error (with -DENABLE_BACKTRACE=OFF (default)):

CALBEG*** ACSCCD -- Version 10.4.0 (07-May-2024) ***
Begin    03-May-2025 18:31:03 EDT
Input    j8cd02tyq_raw.fits
Output   j8cd02tyq_blv_tmp.fits
Warning    Output file `j8cd02tyq_blv_tmp.fits' already exists.
ERROR:    Couldn't process CCD data
ERROR:    CALACS processing NOT completed for j8cd02011_asn.fits
ERROR:    Invalid temporary file. (status = 1021)
DEBUG:    hstcal_error_state_populate called in main() at /home/example/hstcal/pkg/acs/src/mainacs.c:238

And in mainacs:238 we see the following:

int main(int argc, char *argv[]) {
    // ...
            /* Added 19 Mar 1999 - provides interpretation of error for user */
            WhichError (status); // <<< line 238
            freeOnExit(&ptrReg);
            exit (ERROR_RETURN);

This DEBUG message is only visible when NDEBUG is not defined, so this follows the same behavioral pattern as all the assert statements in the code.

Magic numbers have been mapped to the appropriate #define. For any status values that don't map back to a define, I created UNKNOWN_MAGIC_CODE_nnnn defines, and placeholder messages for each. These need to be investigated to see what errors actually need to be emitted.

Fix #187

Comment thread include/hstcalerr.h
Comment thread lib/hstcalerr.c Outdated
Comment thread lib/hstcalerr.c
Comment thread lib/hstcalerr.c
Comment on lines +101 to +102
case INVALID_EXPTIME:
e.runtime.message = "Invalid exposure time.";

@jhunkeler jhunkeler May 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I forgot I made this one up because the message didn't exist. Is this fine, or should it be something else?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

@mdlpstsci mdlpstsci Jun 11, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment for do2d.c Line 414. Either (1) the definition of this case, INVALID_EXPTIME, has to change because the places it was used previously (code 1001?) may not mean what you thought they meant, or (2) use a different code at the error locations.

@jhunkeler jhunkeler force-pushed the error-handler-revamp branch 2 times, most recently from b20e366 to b6d78e2 Compare May 12, 2025 19:01
@jhunkeler jhunkeler force-pushed the error-handler-revamp branch 2 times, most recently from c260529 to 60dfabf Compare May 23, 2025 15:39
@jhunkeler jhunkeler marked this pull request as ready for review June 4, 2025 14:54
@jhunkeler jhunkeler requested a review from a team as a code owner June 4, 2025 14:54
@jhunkeler jhunkeler force-pushed the error-handler-revamp branch from 60dfabf to c913211 Compare June 4, 2025 16:31
Comment thread pkg/acs/lib/acs2d/do2d.c
if (hstio_err()) {
trlerror("Couldn't write imset %d.", extver);
return (status = 1001);
return (status = INVALID_EXPTIME);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EXPSCORR indicates each individual exposure from a CR-SPLIT, repeated sub-exposures, or DITHER observation should generate an FLT/FLC file. In other words the "exps" is misleading in that it does not indicate exposure time, but rather individual exposure calibration (to FLT/FLC). A failure here indicates "WRITE_FAILED". Or at least this is close to what is happening, but it is still not clear why.

@pllim

pllim commented Jul 2, 2025

Copy link
Copy Markdown
Contributor

Lots of conflicts now. If you are still interested in pursuing this, please rebase. Otherwise, please close the PR without merge. Thanks!

@jhunkeler jhunkeler force-pushed the error-handler-revamp branch 2 times, most recently from f130e64 to a2ed2b7 Compare July 3, 2025 13:10
@jhunkeler jhunkeler force-pushed the error-handler-revamp branch from adefbb2 to 3e4e1fb Compare June 2, 2026 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor error handling

4 participants