OpenJPA
  1. OpenJPA
  2. OPENJPA-536

getMetaData() causes OutOfMemoryError under some cases

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.0.2, 1.0.3
    • Fix Version/s: 0.9.7-r547073, 1.0.4, 1.1.0
    • Component/s: jpa
    • Labels:
      None
    • Environment:
      Sun JDK 5

      Description

      There are 3 classes: Item, Artist and Person. Artist extends Person.
      Invoking JPAFacadeHelper.getMetaData(emf, Item.class) will cause OutOfMemoryError thrown by org.apache.openjpa.meta.MetaDataRepository.processBuffer().

      1. OPENJPA-536-2.patch
        0.8 kB
        Amy Yang
      2. OPENJPA-536-1.0.x.patch
        17 kB
        Jody Grassel
      3. OPENJPA-536.patch
        1 kB
        Amy Yang
      4. OPENJPA-536.patch
        16 kB
        Amy Yang
      5. OPENJPA-536.patch
        13 kB
        Amy Yang
      6. OPENJPA-536.patch
        10 kB
        Amy Yang
      7. OPENJPA-536.patch
        11 kB
        Amy Yang
      8. OPENJPA-536.patch
        10 kB
        Amy Yang
      9. OPENJPA-536.patch
        7 kB
        Amy Yang
      10. OPENJPA-536.patch
        9 kB
        Amy Yang
      11. MetaDataRepository.diff
        2 kB
        Amy Yang
      12. meta_testcase.zip
        3 kB
        Amy Yang
      13. InheritanceComparator.diff
        0.5 kB
        Amy Yang
      14. 364202.patch
        151 kB
        Amy Yang

        Activity

        Hide
        Amy Yang added a comment -

        It is because when constructing the TreeSet, the Item will be the root. The handling order will be Item, Person, Artist.
        The the comparator will report Artist < Item and Person > Item. So the TreeSet will be
        Item
        / \
        Artist Person

        Then when traversing the TreeSet and pop the first one, after Item is removed, the TreeSet will be
        Person
        /
        Artist
        The comparator will report Person < Artist so the TreeSet is incorrect.

        Show
        Amy Yang added a comment - It is because when constructing the TreeSet, the Item will be the root. The handling order will be Item, Person, Artist. The the comparator will report Artist < Item and Person > Item. So the TreeSet will be Item / \ Artist Person Then when traversing the TreeSet and pop the first one, after Item is removed, the TreeSet will be Person / Artist The comparator will report Person < Artist so the TreeSet is incorrect.
        Hide
        Amy Yang added a comment -

        The test case can be extracted and run under openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta

        Show
        Amy Yang added a comment - The test case can be extracted and run under openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta
        Hide
        Amy Yang added a comment -

        In InheritanceComparator.java, assign Object.class to _base can make the test case pass. But it will break TestMetaDataInheritanceComparator

        Show
        Amy Yang added a comment - In InheritanceComparator.java, assign Object.class to _base can make the test case pass. But it will break TestMetaDataInheritanceComparator
        Hide
        Amy Yang added a comment -

        Here is a better fix, I think. It can pass both my test case and TestMetaDataInheritanceComparator.
        Basically the comparator doesn't have problem. But when comparing classes, the base class is needed to be set. Otherwise it might only compares the class name.

        Show
        Amy Yang added a comment - Here is a better fix, I think. It can pass both my test case and TestMetaDataInheritanceComparator. Basically the comparator doesn't have problem. But when comparing classes, the base class is needed to be set. Otherwise it might only compares the class name.
        Hide
        Amy Yang added a comment -

        I mean, the changes in MetaDataRepository.diff should be better than InheritanceComparator.diff, though it is not fully reviewed and tested.

        Show
        Amy Yang added a comment - I mean, the changes in MetaDataRepository.diff should be better than InheritanceComparator.diff, though it is not fully reviewed and tested.
        Hide
        Amy Yang added a comment -

        Patch including changed source and test case.

        Show
        Amy Yang added a comment - Patch including changed source and test case.
        Hide
        Albert Lee added a comment -

        Amy,

        First of all, thank you of your contribution to the solution of this problem.

        I assume the last patch (i.e. 364202.patch) contains ALL the artifacts including the solutions and the test case for verification.

        From looking at 364202.patch, it is very difficult to figure out what are the changes in MetaDataRepository.java due to formating differences between the before and after files. If you can submit a patch that shows only the necessary changes, it would greatly speed up the code review process for the committer to help you to commit the change.

        Thanks,
        Albert Lee.

        Show
        Albert Lee added a comment - Amy, First of all, thank you of your contribution to the solution of this problem. I assume the last patch (i.e. 364202.patch) contains ALL the artifacts including the solutions and the test case for verification. From looking at 364202.patch, it is very difficult to figure out what are the changes in MetaDataRepository.java due to formating differences between the before and after files. If you can submit a patch that shows only the necessary changes, it would greatly speed up the code review process for the committer to help you to commit the change. Thanks, Albert Lee.
        Hide
        Albert Lee added a comment -

        Following the instructions ( http://openjpa.apache.org/get-involved.html ) and the coding standard ( http://openjpa.apache.org/coding-standards.html ) help to eliminate the situation you ran into.

        Thanks,
        Albert Lee.

        Show
        Albert Lee added a comment - Following the instructions ( http://openjpa.apache.org/get-involved.html ) and the coding standard ( http://openjpa.apache.org/coding-standards.html ) help to eliminate the situation you ran into. Thanks, Albert Lee.
        Hide
        Amy Yang added a comment -

        Thank you Albert.
        I generated another patch which is the output of "svn diff openjpa-kernel\src\main\java\org\apache\openjpa\meta\MetaDataRepository.java". Hope you can review it.
        And I executed "svn add" for test cases:
        svn add openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Person.java openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Item.java openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Artist.java openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\TestGetMetaData.java
        A openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Person.java
        A openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Item.java
        A openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Artist.java
        A openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\TestGetMetaData.java

        Please correct me if anything is wrong.

        Show
        Amy Yang added a comment - Thank you Albert. I generated another patch which is the output of "svn diff openjpa-kernel\src\main\java\org\apache\openjpa\meta\MetaDataRepository.java". Hope you can review it. And I executed "svn add" for test cases: svn add openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Person.java openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Item.java openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Artist.java openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\TestGetMetaData.java A openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Person.java A openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Item.java A openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\Artist.java A openjpa-persistence-jdbc\src\test\java\org\apache\openjpa\meta\TestGetMetaData.java Please correct me if anything is wrong.
        Hide
        Amy Yang added a comment -

        try to set the base of comparator.

        Show
        Amy Yang added a comment - try to set the base of comparator.
        Hide
        Amy Yang added a comment -

        try to set the base of comparator

        Show
        Amy Yang added a comment - try to set the base of comparator
        Hide
        Amy Yang added a comment -

        oops. pasted MetaDataRepository.java. Please ignore it. only OPENJPA-536.patch is ok, I think.

        Why can't I delete the attachments? :-P

        Show
        Amy Yang added a comment - oops. pasted MetaDataRepository.java. Please ignore it. only OPENJPA-536 .patch is ok, I think. Why can't I delete the attachments? :-P
        Hide
        Pinaki Poddar added a comment -

        Amy,
        Thanks for this critical patch.

        It may by useful to name your patch as OPENJPA-XYZ.n.patch
        where XYZ is the JIRA issue number
        n is the serial number of the patch as often multiple patches are submitted by the same/different developer(s)

        Show
        Pinaki Poddar added a comment - Amy, Thanks for this critical patch. It may by useful to name your patch as OPENJPA-XYZ.n.patch where XYZ is the JIRA issue number n is the serial number of the patch as often multiple patches are submitted by the same/different developer(s)
        Hide
        Amy Yang added a comment -

        Thank you Pinaki.
        I'll remember the serial number.

        Hope the fix is right. Will you review it?

        Show
        Amy Yang added a comment - Thank you Pinaki. I'll remember the serial number. Hope the fix is right. Will you review it?
        Hide
        Amy Yang added a comment -

        patch including source code diff and test case

        Show
        Amy Yang added a comment - patch including source code diff and test case
        Hide
        Amy Yang added a comment -

        This version of patch can pass both my test case and the remote test on TeamCity.
        Please help to review it.

        Show
        Amy Yang added a comment - This version of patch can pass both my test case and the remote test on TeamCity. Please help to review it.
        Hide
        Patrick Linskey added a comment -

        Resolved with r644115. For the record, I'm not in love with the statefulness involved in this algorithm, but that predates this issue.

        Show
        Patrick Linskey added a comment - Resolved with r644115. For the record, I'm not in love with the statefulness involved in this algorithm, but that predates this issue.
        Hide
        Amy Yang added a comment -

        My previous fix doesn't resolve the problem completely.

        I'll paste another one.

        Show
        Amy Yang added a comment - My previous fix doesn't resolve the problem completely. I'll paste another one.
        Hide
        Amy Yang added a comment -

        This is the latest fix.
        Please help to review it.

        Show
        Amy Yang added a comment - This is the latest fix. Please help to review it.
        Hide
        Amy Yang added a comment -

        this is the patch under branch 1.1.x.

        it's a little weird that my subversion is a Chinese version. I don't know if the patch can be applied...

        Show
        Amy Yang added a comment - this is the patch under branch 1.1.x. it's a little weird that my subversion is a Chinese version. I don't know if the patch can be applied...
        Hide
        Amy Yang added a comment -

        a newer patch for 1.1.x

        Show
        Amy Yang added a comment - a newer patch for 1.1.x
        Hide
        Amy Yang added a comment -

        Modification of the patch:
        1. use an internal LinkedList instead of involving a new class
        2. add a private method addToBuffer() to MetaDataRepository to enable MetaDataInheritanceComparator.compare() when adding to buffer
        3. use default remove() of LinkedList
        4. set the default value of InheritanceComparator._base to Object.class
        5. add one test case testGetMetaData. delete 2 test methods from TestMetaDataInheritanceComparator: testInheritanceComparatorWithoutBase() and testMetaDataInheritanceComparatorWithoutBase().

        Please help to review.

        Show
        Amy Yang added a comment - Modification of the patch: 1. use an internal LinkedList instead of involving a new class 2. add a private method addToBuffer() to MetaDataRepository to enable MetaDataInheritanceComparator.compare() when adding to buffer 3. use default remove() of LinkedList 4. set the default value of InheritanceComparator._base to Object.class 5. add one test case testGetMetaData. delete 2 test methods from TestMetaDataInheritanceComparator: testInheritanceComparatorWithoutBase() and testMetaDataInheritanceComparatorWithoutBase(). Please help to review.
        Hide
        Amy Yang added a comment -

        modified patch.

        Show
        Amy Yang added a comment - modified patch.
        Hide
        Abe White added a comment -

        Latest patch looks pretty good. Two minor details:
        1. The class should just be InheritanceOrderMetaDataList, not InheritanceOrderMetaDataList<ClassMetaData>.
        2. You don't need all the "if (buffer != null)" checks. buffer is never null. In fact you could make it final.

        Show
        Abe White added a comment - Latest patch looks pretty good. Two minor details: 1. The class should just be InheritanceOrderMetaDataList, not InheritanceOrderMetaDataList<ClassMetaData>. 2. You don't need all the "if (buffer != null)" checks. buffer is never null. In fact you could make it final.
        Hide
        Amy Yang added a comment -

        a little improvement

        Show
        Amy Yang added a comment - a little improvement
        Hide
        Patrick Linskey added a comment -

        I think that we can put this one to rest finally.

        Show
        Patrick Linskey added a comment - I think that we can put this one to rest finally.
        Hide
        Jody Grassel added a comment -

        Patch to bring this bugfix back into the 1.0.x stream.

        Show
        Jody Grassel added a comment - Patch to bring this bugfix back into the 1.0.x stream.
        Hide
        Michael Dick added a comment -

        Thanks for the patch Joe.

        Show
        Michael Dick added a comment - Thanks for the patch Joe.
        Hide
        Michael Dick added a comment -

        Closing this issue on Amy's behalf. It's fixed in all applicable releases.

        Show
        Michael Dick added a comment - Closing this issue on Amy's behalf. It's fixed in all applicable releases.

          People

          • Assignee:
            Unassigned
            Reporter:
            Amy Yang
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development