Uploaded image for project: 'OpenJPA'
  1. OpenJPA
  2. OPENJPA-2507

Weird EmptyStackException in CriteriaQueryImpl

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.0, 2.3.0, 2.4.0
    • Fix Version/s: 2.4.0
    • Component/s: criteria
    • Environment:
      openjpa 2.2.0 & 2.3.0, spring-data-jpa up to 1.4.2

      Description

      Using spring-data-jpa with openjpa, I sometimes encounter a strange error (not all the times, but under heavy load it makes its appearance, sparsely though):
      ....
      Caused by: java.util.EmptyStackException
      at java.util.Stack.peek(Stack.java:<arbitrary line>)
      at org.apache.openjpa.persistence.criteria.CriteriaQueryImpl.ctx(CriteriaQueryImpl.java:<arbitrary line>
      ....

      I do not know which behaviour triggers it, however I think it would be an improvement to change (I did not know where exactly to file it, because it is both an improvement and a bug in my opinion), in org.apache.openjpa.persistence.criteria.CriteriaQueryImpl, method: Context ctx(), this:
      return _contexts == null || _contexts.isEmpty() ? null : _contexts.peek();
      to something like this:
      try

      { return _contexts == null || _contexts.isEmpty() ? null : _contexts.peek(); }

      catch (EmptyStackException e)

      { return null; }

      , in order to prevent a case where multiple threads modify the "_contexts" between the evaluation of the inline "if".

      I am not able to reproduce it all the time, thus I can't create a useful test, neither have I created a patch due to the simplicity of the 'fix'. However I believe it is a harmless fix which could be considered a minor improvement.

      1. exceptions.txt
        16 kB
        David Bilge
      2. OPENJPA-2507-Wrap-ContextStack-in-ThreadLocal.patch
        3 kB
        Thomas Darimont
      3. openjpa-exception-demo.zip
        7 kB
        David Bilge

        Issue Links

          Activity

          Hide
          kwsutter Kevin Sutter added a comment -

          Although I understand the logic behind your suggestion – to prevent the EmptyStackException – I don't necessarily agree with the reasoning for this change. If multiple threads are getting into this method, then I think we have bigger issues with this class. None of the instance variables are protected for multiple thread access. So, if we're having issues with the Stacks, we should be hitting issues with the Maps, and Lists, and other collections.

          Given that, I also found an older JIRA (openjpa-2098) which seems to have hit a similar issue. Pinaki had resolved that issue by putting in the null and isEmpty() checks that you are referencing. So, it seems we are putting band-aids on this issue without fully understanding the source.

          The common link between the two scenarios is the use of the spring-data-jpa feature. Not pointing fingers at this, just highlighting the common aspect of the two descriptions.

          Show
          kwsutter Kevin Sutter added a comment - Although I understand the logic behind your suggestion – to prevent the EmptyStackException – I don't necessarily agree with the reasoning for this change. If multiple threads are getting into this method, then I think we have bigger issues with this class. None of the instance variables are protected for multiple thread access. So, if we're having issues with the Stacks, we should be hitting issues with the Maps, and Lists, and other collections. Given that, I also found an older JIRA (openjpa-2098) which seems to have hit a similar issue. Pinaki had resolved that issue by putting in the null and isEmpty() checks that you are referencing. So, it seems we are putting band-aids on this issue without fully understanding the source. The common link between the two scenarios is the use of the spring-data-jpa feature. Not pointing fingers at this, just highlighting the common aspect of the two descriptions.
          Hide
          ialex Ioannis Alexandrakis added a comment -

          I have not seen the source of spring-data very closely to see if it caches CriteriaQueryImpl (even though after your comment I suspect that that is the case, or something of the sort, like modifying the contexts using get/setContexts), but since this class was never intended to be accessed by multiple threads, then there is no reason to protect that part of the class only.

          However, Stacks' methods are synchronized and there is only a setter for contexts (these are the main differences from the other Maps/Lists inside CriteriaQueryImpl), perhaps if there is an explanation to the problem would be related to those. I am not saying this is worth the effort, but I am just curious as to why we have a problem with the specific method (Context ctx()) in two tickets which are probably related.

          Just to mention it, since I tested the proposed fix myself, I haven't seen something related to CriteriaQueryImpl again.

          Show
          ialex Ioannis Alexandrakis added a comment - I have not seen the source of spring-data very closely to see if it caches CriteriaQueryImpl (even though after your comment I suspect that that is the case, or something of the sort, like modifying the contexts using get/setContexts), but since this class was never intended to be accessed by multiple threads, then there is no reason to protect that part of the class only. However, Stacks' methods are synchronized and there is only a setter for contexts (these are the main differences from the other Maps/Lists inside CriteriaQueryImpl), perhaps if there is an explanation to the problem would be related to those. I am not saying this is worth the effort, but I am just curious as to why we have a problem with the specific method (Context ctx()) in two tickets which are probably related. Just to mention it, since I tested the proposed fix myself, I haven't seen something related to CriteriaQueryImpl again.
          Hide
          kwsutter Kevin Sutter added a comment -

          Interesting. The more I look into this default Stack implementation, it doesn't look to be very solid. Even in the Javadoc for Stack, it recommends the use of Deque instead:

          "A more complete and consistent set of LIFO stack operations is provided by the Deque[1] interface and its implementations, which should be used in preference to this class. For example:

          Deque<Integer> stack = new ArrayDeque<Integer>();"

          [1] http://docs.oracle.com/javase/7/docs/api/java/util/Deque.html

          And, looking at the Deque's peek operation, it doesn't throw an exception if it's empty, it just returns null. It might be better to just use the Deque collection instead of Stack... Of course, this would have a larger ripple effect in the code base...

          It's good to know that your proposed hack seems to resolve the issue for you. Maybe that's what we need to do for the time being and worry about the "larger fish" at a later time... I have also reached out to Pinaki to see if he has any preference.

          Show
          kwsutter Kevin Sutter added a comment - Interesting. The more I look into this default Stack implementation, it doesn't look to be very solid. Even in the Javadoc for Stack, it recommends the use of Deque instead: "A more complete and consistent set of LIFO stack operations is provided by the Deque [1] interface and its implementations, which should be used in preference to this class. For example: Deque<Integer> stack = new ArrayDeque<Integer>();" [1] http://docs.oracle.com/javase/7/docs/api/java/util/Deque.html And, looking at the Deque's peek operation, it doesn't throw an exception if it's empty, it just returns null. It might be better to just use the Deque collection instead of Stack... Of course, this would have a larger ripple effect in the code base... It's good to know that your proposed hack seems to resolve the issue for you. Maybe that's what we need to do for the time being and worry about the "larger fish" at a later time... I have also reached out to Pinaki to see if he has any preference.
          Hide
          ialex Ioannis Alexandrakis added a comment - - edited

          My initial thought was that since the Deque interface was introduced in java 6, there might be compatibility reasons as to why someone would choose Stack over Deque.

          Glad to be (even of minor) help

          Show
          ialex Ioannis Alexandrakis added a comment - - edited My initial thought was that since the Deque interface was introduced in java 6, there might be compatibility reasons as to why someone would choose Stack over Deque. Glad to be (even of minor) help
          Hide
          oliver.gierke Oliver Gierke added a comment - - edited

          Just to confirm this: Spring Data JPA is caching CriteriaQuery instances as - in contrast to EntityManager and EntityManagerFactory - there's no hint in the spec about the CriteriaQuery instances to be allowed to not be thread safe, once constructed and equipped with predicates.

          Show
          oliver.gierke Oliver Gierke added a comment - - edited Just to confirm this: Spring Data JPA is caching CriteriaQuery instances as - in contrast to EntityManager and EntityManagerFactory - there's no hint in the spec about the CriteriaQuery instances to be allowed to not be thread safe, once constructed and equipped with predicates.
          Hide
          viadavid David Bilge added a comment - - edited

          I experienced this very bug and created a demo project (see attached openjpa-exception-demo.zip) - if that helps. Just execute LoadTest#parallelTest and invariably one of the exceptions in exceptions.txt will be thrown. One of those is the EmptyStackException.

          It is easy to reproduce this without Spring Data by using this alternative implementation for the DemoRepository interface from the demo project:

          @Component
          public class ManualDemoRepository implements DemoRepository {
          
          	@Autowired
          	EntityManager entityManager;
          
          	CriteriaQuery<DemoEntity> criteriaQuery;
          
          	@PostConstruct
          	public void initQuery() {
          		CriteriaBuilder criteriaBuilder = entityManager.getCriteriaBuilder();
          		criteriaQuery = criteriaBuilder.createQuery(DemoEntity.class);
          
          		Root<DemoEntity> demoRoot = criteriaQuery.from(DemoEntity.class);
          
          		criteriaQuery.select(demoRoot).where(criteriaBuilder.equal(demoRoot.get(DemoEntity_.demoNumber), 42));
          	}
          
          	@Override
          	public DemoEntity findByDemoNumber(int demoNumber) {
          		return entityManager.createQuery(criteriaQuery).getSingleResult();
          	}
          }
          
          Show
          viadavid David Bilge added a comment - - edited I experienced this very bug and created a demo project (see attached openjpa-exception-demo.zip ) - if that helps. Just execute LoadTest#parallelTest and invariably one of the exceptions in exceptions.txt will be thrown. One of those is the EmptyStackException . It is easy to reproduce this without Spring Data by using this alternative implementation for the DemoRepository interface from the demo project: @Component public class ManualDemoRepository implements DemoRepository { @Autowired EntityManager entityManager; CriteriaQuery<DemoEntity> criteriaQuery; @PostConstruct public void initQuery() { CriteriaBuilder criteriaBuilder = entityManager.getCriteriaBuilder(); criteriaQuery = criteriaBuilder.createQuery(DemoEntity.class); Root<DemoEntity> demoRoot = criteriaQuery.from(DemoEntity.class); criteriaQuery.select(demoRoot).where(criteriaBuilder.equal(demoRoot.get(DemoEntity_.demoNumber), 42)); } @Override public DemoEntity findByDemoNumber( int demoNumber) { return entityManager.createQuery(criteriaQuery).getSingleResult(); } }
          Hide
          thomasd Thomas Darimont added a comment -

          Hello,

          I just had a look at this and I think a solution that fixes this issue.

          I noticed by debugging the provided example app that an instance of org.apache.openjpa.persistence.criteria.CriteriaQueryImpl is shared between multiple instances of org.apache.openjpa.jdbc.kernel.JDBCStoreQuery. In CriteriaQueryImpl the _contexts field is read and written by multiple threads.

          I think the problem lies in org.apache.openjpa.persistence.criteria.CriteriaQueryImpl.getQueryExpressions(ExpressionFactory):

             QueryExpressions getQueryExpressions(ExpressionFactory factory) {
                  _contexts = new Stack<Context>();
                  Context context = new Context(null, null, null);
                  _contexts.push(context);
                  return new CriteriaExpressionBuilder().getQueryExpressions(factory, this);
              }   
          

          Since the (shared) _context field is modified and read by multiple threads this causes the spurious NPE / EmptyStackExceptions to appear.
          One (relatively easy) way to solve this would be to just wrap the _context in a ThreadLocal - I added a patch file that does exactly that. Note that we remove the stored ThreadLocal value when we leave getQueryExpressions

          With this fixed I could run the provided example app without any problems. This patch would work for openjpa 2.3.0 as well as 2.4.0.
          I tested this with openjpa 2.3.0 directly. I created the patch from the latest svn head for 2.4.0.

          Cheers,
          Thomas

          Show
          thomasd Thomas Darimont added a comment - Hello, I just had a look at this and I think a solution that fixes this issue. I noticed by debugging the provided example app that an instance of org.apache.openjpa.persistence.criteria.CriteriaQueryImpl is shared between multiple instances of org.apache.openjpa.jdbc.kernel.JDBCStoreQuery . In CriteriaQueryImpl the _contexts field is read and written by multiple threads. I think the problem lies in org.apache.openjpa.persistence.criteria.CriteriaQueryImpl.getQueryExpressions(ExpressionFactory) : QueryExpressions getQueryExpressions(ExpressionFactory factory) { _contexts = new Stack<Context>(); Context context = new Context( null , null , null ); _contexts.push(context); return new CriteriaExpressionBuilder().getQueryExpressions(factory, this ); } Since the (shared) _context field is modified and read by multiple threads this causes the spurious NPE / EmptyStackExceptions to appear. One (relatively easy) way to solve this would be to just wrap the _context in a ThreadLocal - I added a patch file that does exactly that. Note that we remove the stored ThreadLocal value when we leave getQueryExpressions With this fixed I could run the provided example app without any problems. This patch would work for openjpa 2.3.0 as well as 2.4.0. I tested this with openjpa 2.3.0 directly. I created the patch from the latest svn head for 2.4.0. Cheers, Thomas
          Hide
          thomasd Thomas Darimont added a comment -

          Patch that wraps the _context field in org.apache.openjpa.persistence.criteria.CriteriaQueryImpl<T> in a ThreadLocal.

          Show
          thomasd Thomas Darimont added a comment - Patch that wraps the _context field in org.apache.openjpa.persistence.criteria.CriteriaQueryImpl<T> in a ThreadLocal.
          Hide
          kwsutter Kevin Sutter added a comment -

          Thank you, Thomas. I like this approach the best thus far. I agree with where you think the problem originates. And, I didn't like the idea of changing the data structure itself due to the ripple effect to other parts. But, wrapping the Stack in a ThreadLocal seems to be an easy, safe mechanism to overcome the problem. I want to experiment and look at the patch a bit more to ensure that we're removing this ThreadLocal appropriately, otherwise we could end up with a leak.

          Thanks, Kevin

          Show
          kwsutter Kevin Sutter added a comment - Thank you, Thomas. I like this approach the best thus far. I agree with where you think the problem originates. And, I didn't like the idea of changing the data structure itself due to the ripple effect to other parts. But, wrapping the Stack in a ThreadLocal seems to be an easy, safe mechanism to overcome the problem. I want to experiment and look at the patch a bit more to ensure that we're removing this ThreadLocal appropriately, otherwise we could end up with a leak. Thanks, Kevin
          Hide
          kwsutter Kevin Sutter added a comment -

          With some minor modifications, the patch provided by Thomas looks good. I've reviewed the code and experimented with it and have not reproduced the problem either. I plan to commit this change to trunk (2.4.0) shortly.

          Show
          kwsutter Kevin Sutter added a comment - With some minor modifications, the patch provided by Thomas looks good. I've reviewed the code and experimented with it and have not reproduced the problem either. I plan to commit this change to trunk (2.4.0) shortly.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1601778 from Kevin Sutter in branch 'openjpa/trunk'
          [ https://svn.apache.org/r1601778 ]

          OPENJPA-2507. Committing a variation of Thomas Darimont's patch that utilizes ThreadLocal storage to safeguard the _contexts in CriteriaQueryImpl.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1601778 from Kevin Sutter in branch 'openjpa/trunk' [ https://svn.apache.org/r1601778 ] OPENJPA-2507 . Committing a variation of Thomas Darimont's patch that utilizes ThreadLocal storage to safeguard the _contexts in CriteriaQueryImpl.
          Hide
          viadavid David Bilge added a comment - - edited

          That's great news! Are there any plans about when 2.4 will be released?

          Show
          viadavid David Bilge added a comment - - edited That's great news! Are there any plans about when 2.4 will be released?
          Hide
          kwsutter Kevin Sutter added a comment - - edited

          No plans currently. We're working through some issues with Java 7, Java 8, ASM, and Serp on trunk (2.4.0)... Once these items get addressed, then we could entertain a possible 2.4.0 release. To be honest, if you require this change in a formal release, you might want to pursue this fix in one of the service streams and pushing for a service release. Thanks.

          Show
          kwsutter Kevin Sutter added a comment - - edited No plans currently. We're working through some issues with Java 7, Java 8, ASM, and Serp on trunk (2.4.0)... Once these items get addressed, then we could entertain a possible 2.4.0 release. To be honest, if you require this change in a formal release, you might want to pursue this fix in one of the service streams and pushing for a service release. Thanks.

            People

            • Assignee:
              kwsutter Kevin Sutter
              Reporter:
              ialex Ioannis Alexandrakis
            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development