Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-9792

Mapping::getInverse not exception-safe

    XMLWordPrintable

Details

    Description

      The current implementation of Mapping::getInverse allocates a new AstMapping, but does not free it if AST's internal error flag triggers an exception via assertOK. In addition, Object::fromAstObject does not free rawObjCopy if there is a dynamic casting error.

      The leaks in these methods should be fixed using either assertOK or a catch or finally block. Unfortunately, as I understand it astshim's object and type juggling would interfere with using a unique_ptr.

      In addition, both methods should document the exceptions they throw and their (intended) exception safety.

      Attachments

        Activity

          rowen Russell Owen added a comment -

          Almost all methods of astshim may throw std::runtime_error if AST reports an error. It is fine to document the most commonly expected errors, but we make promises that those are the only reasons code can throw. If nothing else, it is always possible to call AST code directly and put it into an error state; then the next call to astshim that checks state will throw (not that we expect users to be calling AST code directly). Similarly, if astshim forgets to check in one bit of code then this can manifest as an unexpected error in another bit of code.

          I really don't want to clutter up astshim's documentation with "this can throw for any reason" boilerplate. Please keep it small; stick to exceptions that can reasonably be expected. And please do not document "can run out of memory" for a cheap operation like duplicating a transform. Any object creation can run out of memory, but it's not likely and it adds needless clutter to call it out.

          rowen Russell Owen added a comment - Almost all methods of astshim may throw std::runtime_error if AST reports an error. It is fine to document the most commonly expected errors, but we make promises that those are the only reasons code can throw. If nothing else, it is always possible to call AST code directly and put it into an error state; then the next call to astshim that checks state will throw (not that we expect users to be calling AST code directly). Similarly, if astshim forgets to check in one bit of code then this can manifest as an unexpected error in another bit of code. I really don't want to clutter up astshim's documentation with "this can throw for any reason" boilerplate. Please keep it small; stick to exceptions that can reasonably be expected. And please do not document "can run out of memory" for a cheap operation like duplicating a transform. Any object creation can run out of memory, but it's not likely and it adds needless clutter to call it out.
          rowen Russell Owen added a comment -

          I have discovered a bug in PolyMap that I have sent to David Berry. I hope to be able to wait for that to be fixed, and provide a test for it as part of this ticket, before finishing this ticket.

          rowen Russell Owen added a comment - I have discovered a bug in PolyMap that I have sent to David Berry. I hope to be able to wait for that to be fixed, and provide a test for it as part of this ticket, before finishing this ticket.

          The potential memory leak in getInverse is fixed.

          Other changes that also made on this ticket:

          • I improved the documentation for Mapping.getInverse
          • I renamed Mapping.getTranForward to Mapping.hasTranForward and Mapping.getTranInverse to Mapping.hasTranInverse
          • Minor improvements to the doc/mainpage.dox
          • I beefed up test_polyMap.py to check mappings with no inverse (thus hasTranForward and hasTranInverse), to check the iterative inverse, and to check iterative inverse properties as attributes for the constructor that only takes coefficients for the forward transform.

          I have not yet figured out how to better document the fact that nearly any method can, in theory, raise std::runtime_error. It is mentioned in mainpage.dox but not as clearly as it probably should be. I think improving that is part of a larger task to improve the introductory documentation.

          Note that the test_polyMap.py improvements exposed a bug in AST which is fixed in version 8.5.0. Thus you will need to update starlink_ast in order to use this version of astshim.

          rowen Russell Owen added a comment - The potential memory leak in getInverse is fixed. Other changes that also made on this ticket: I improved the documentation for Mapping.getInverse I renamed Mapping.getTranForward to Mapping.hasTranForward and Mapping.getTranInverse to Mapping.hasTranInverse Minor improvements to the doc/mainpage.dox I beefed up test_polyMap.py to check mappings with no inverse (thus hasTranForward and hasTranInverse ), to check the iterative inverse, and to check iterative inverse properties as attributes for the constructor that only takes coefficients for the forward transform. I have not yet figured out how to better document the fact that nearly any method can, in theory, raise std::runtime_error . It is mentioned in mainpage.dox but not as clearly as it probably should be. I think improving that is part of a larger task to improve the introductory documentation. Note that the test_polyMap.py improvements exposed a bug in AST which is fixed in version 8.5.0. Thus you will need to update starlink_ast in order to use this version of astshim .

          Re: prominence, I think the main problem is that the "main" page is kind of buried (I only found https://lsst-web.ncsa.illinois.edu/doxygen/x_masterDoxyDoc/astshim.html because you just told me about it). Until DMTN-030 is implemented, a user is unlikely to notice anything above class level. (Not saying you should add notes to every class, just that there is a fundamental scoping issue that's not coming from astshim itself.)

          krzys Krzysztof Findeisen added a comment - Re: prominence, I think the main problem is that the "main" page is kind of buried (I only found https://lsst-web.ncsa.illinois.edu/doxygen/x_masterDoxyDoc/astshim.html because you just told me about it). Until DMTN-030 is implemented, a user is unlikely to notice anything above class level. (Not saying you should add notes to every class, just that there is a fundamental scoping issue that's not coming from astshim itself.)

          Some typos and similar corrections on GitHub, otherwise looks good.

          krzys Krzysztof Findeisen added a comment - Some typos and similar corrections on GitHub, otherwise looks good.

          People

            rowen Russell Owen
            krzys Krzysztof Findeisen
            Krzysztof Findeisen
            Krzysztof Findeisen, Russell Owen, Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Jenkins

                No builds found.