OpenJPA
  1. OpenJPA
  2. OPENJPA-251

org.apache.openjpa.enhance.Reflection.getDeclaredMethod() has undefined behavior, leading to VM-dependent crashes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.9.7
    • Fix Version/s: 1.0.2, 1.1.0
    • Component/s: jpa
    • Labels:
      None
    • Environment:
      Sun JDK 6.01

      Description

      Given

      public interface A

      { Object getId(); }

      @Entity
      public class B implements A {
      @Id
      public String getId()

      { return "foo"; }


      }

      B.class.getDeclaredMethods() will include both "public java.lang.String B.getId()" and "public java.lang.Object B.getId()". The order in which these two methods appear is NOT DEFINED! Because org.apache.openjpa.enhance.Reflection.getDeclaredMethod() returns the first matching method, and because that method might well be the abstract one retuning Object, OpenJPA will complain that it cannot persist an ID with a non-explicit strategy, and throw up.

      Class.getDeclaredMethod() (note singular, not plural) is defined to return the method with the most specific return type under these circumstances, and should therefore be used. Here's my implementation of Reflection.getDeclaredMethod:

      private static Method getDeclaredMethod(Class cls, String name, Class param)
      {
      Class[] params = param == null ? new Class[0] : new Class[]

      { param }

      ;
      try

      { return cls.getDeclaredMethod(name, params); }

      catch (Exception e)

      { return null; }

      }

      1. weirdness.zip
        2 kB
        Jonathan Feinberg

        Issue Links

          Activity

          Hide
          Jonathan Feinberg added a comment -

          The enclosed demonstration program gives the output

          public java.lang.String weirdness.TheImplementation.getIt()
          public java.lang.Object weirdness.TheImplementation.getIt()

          on my machine, but gives the output

          public java.lang.String weirdness.TheImplementation.getIt()
          public java.lang.String weirdness.TheImplementation.getIt()

          on a seemingly identical machine belonging to my colleague.

          Show
          Jonathan Feinberg added a comment - The enclosed demonstration program gives the output public java.lang.String weirdness.TheImplementation.getIt() public java.lang.Object weirdness.TheImplementation.getIt() on my machine, but gives the output public java.lang.String weirdness.TheImplementation.getIt() public java.lang.String weirdness.TheImplementation.getIt() on a seemingly identical machine belonging to my colleague.
          Hide
          Craig L Russell added a comment -

          Thanks for the info, Jonathan. Seems like a good analysis and an easy fix.

          Would you be able to provide a patch, and run the build script to verify it doesn't break anything?

          Show
          Craig L Russell added a comment - Thanks for the info, Jonathan. Seems like a good analysis and an easy fix. Would you be able to provide a patch, and run the build script to verify it doesn't break anything?
          Hide
          Patrick Linskey added a comment -

          From a fix standpoint, it'd be nice to come up with a helper method that replaces the functionality of getDeclaredMethods(), but only lists the most-specific of duplicate covariant signatures (getMostSpecificDeclaredMethods() or something) so that we can use it throughout the codebase without duplication of the looping logic.

          Show
          Patrick Linskey added a comment - From a fix standpoint, it'd be nice to come up with a helper method that replaces the functionality of getDeclaredMethods(), but only lists the most-specific of duplicate covariant signatures (getMostSpecificDeclaredMethods() or something) so that we can use it throughout the codebase without duplication of the looping logic.
          Hide
          Abe White added a comment -

          The original implementation used Class.getDeclaredMethod. It was changed because this is significantly slower than using getDeclaredMethods() and searching through them, and this method is used a lot during deployment.

          Show
          Abe White added a comment - The original implementation used Class.getDeclaredMethod. It was changed because this is significantly slower than using getDeclaredMethods() and searching through them, and this method is used a lot during deployment.
          Hide
          Jonathan Feinberg added a comment -

          Abe, I'd rather have the right answer slowly.

          Out of curiosity, how much time does it add to deployment?

          Show
          Jonathan Feinberg added a comment - Abe, I'd rather have the right answer slowly. Out of curiosity, how much time does it add to deployment?
          Hide
          Patrick Linskey added a comment -

          It was significant, due to the additional exceptions that were raised by getDeclaredMethod(), which throws when it fails.

          We should aim to have a solution that is both correct and fast, IMO. It shouldn't be that hard to implement such a solution with appropriate use of a bit of caching. As ever, it's unfortunate to need to add a cache, since that's one more memory resource to manage and synchronization area to care about.

          In fact, IIRC, a caching solution was marginally faster than the non-caching solution that we ended up using.

          Show
          Patrick Linskey added a comment - It was significant, due to the additional exceptions that were raised by getDeclaredMethod(), which throws when it fails. We should aim to have a solution that is both correct and fast, IMO. It shouldn't be that hard to implement such a solution with appropriate use of a bit of caching. As ever, it's unfortunate to need to add a cache, since that's one more memory resource to manage and synchronization area to care about. In fact, IIRC, a caching solution was marginally faster than the non-caching solution that we ended up using.
          Hide
          Marc Prud'hommeaux added a comment -

          Bumping to release 1.0.1 since 1.0.0 is being released.

          Show
          Marc Prud'hommeaux added a comment - Bumping to release 1.0.1 since 1.0.0 is being released.
          Hide
          Albert Lee added a comment -

          Defer to next release.

          Show
          Albert Lee added a comment - Defer to next release.
          Hide
          Patrick Linskey added a comment -

          These seem like they might be related.

          Show
          Patrick Linskey added a comment - These seem like they might be related.
          Hide
          Patrick Linskey added a comment -

          I was unable to reproduce this on my machine (presumably my VM uses the "expected" ordering), but I believe that I resolved the issue with changes to getDeclaredMethod().

          Show
          Patrick Linskey added a comment - I was unable to reproduce this on my machine (presumably my VM uses the "expected" ordering), but I believe that I resolved the issue with changes to getDeclaredMethod().

            People

            • Assignee:
              Unassigned
              Reporter:
              Jonathan Feinberg
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development