Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1816

JaninoRelMetadataProvider generated classes leak ACTIVE nodes on exception

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.13.0
    • Fix Version/s: 1.13.0
    • Component/s: core
    • Labels:
      None

      Description

      The exception catch in the GeneratedMetadataHandler_XXX removes the ACTIVE guard node in the mq.map cache only if the exception is of type NoHandler. For any other exception (notably for CyclicMetadataException) the ACTIVE node is leaked (left in the map). CyclicMetadataException exceptions are frequent because of cycles in RelSubset.

      This issue is not visible normally because the aggressive refresh of mq via RelMetadaatQuery.instance() calls (CALCITE-1812). With my changes to increase the lifetime of the used mq instances the leaked nodes become a problem as they trigger bogus CyclicMetadataException from leftover nodes.

        Issue Links

          Activity

          Show
          rusanu Remus Rusanu added a comment - https://github.com/apache/calcite/pull/461
          Hide
          julianhyde Julian Hyde added a comment -

          Isn't catch (Throwable) always a bad idea?

          Show
          julianhyde Julian Hyde added a comment - Isn't catch (Throwable) always a bad idea?
          Hide
          rusanu Remus Rusanu added a comment -

          I guess it is, this is one of Java subtler points I don't have much practical experience. How about a finally with a success flag, that should cover all cases.

          Show
          rusanu Remus Rusanu added a comment - I guess it is, this is one of Java subtler points I don't have much practical experience. How about a finally with a success flag, that should cover all cases.
          Hide
          julianhyde Julian Hyde added a comment -

          Probably the ideal is catch (Exception). Then check the surrounding code to make sure that an Error will cause everything to be destroyed (i.e. it isn't incorrectly caught).

          Show
          julianhyde Julian Hyde added a comment - Probably the ideal is catch (Exception) . Then check the surrounding code to make sure that an Error will cause everything to be destroyed (i.e. it isn't incorrectly caught).
          Hide
          rusanu Remus Rusanu added a comment -

          Exception it is then. I refreshed the PR.

          Show
          rusanu Remus Rusanu added a comment - Exception it is then. I refreshed the PR.
          Hide
          julianhyde Julian Hyde added a comment -
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/d4b0acdd . Thanks for the PR, Remus Rusanu !
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.13.0 (2017-06-26).

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.13.0 (2017-06-26).

            People

            • Assignee:
              rusanu Remus Rusanu
              Reporter:
              rusanu Remus Rusanu
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development