Bug 1014524 - CriteriaQuery Iterator #next() throws java.util.NoSuchElementException
CriteriaQuery Iterator #next() throws java.util.NoSuchElementException
Status: CLOSED CURRENTRELEASE
Product: RHQ Project
Classification: Other
Component: Core Server (Show other bugs)
4.9
Unspecified Unspecified
medium Severity high (vote)
: ---
: RHQ 4.10
Assigned To: RHQ Project Maintainer
Mike Foley
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-10-02 05:35 EDT by Thomas Segismont
Modified: 2014-04-23 08:31 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-04-23 08:31:59 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Thomas Segismont 2013-10-02 05:35:32 EDT
CriteriaQuery Iterator #next() may throw java.util.NoSuchElementException even after a successful #hasNext() call

See details in BZ1014300. In particular:

(Lukas Krejci comment #2)
> The NSEE is thrown on line CriteriaQuery.java#145, which is a call to the
> underlying iterator.next(). This is directly preceeded by an "if
> (!iterator.hasNext())" branch.
> 
> So the only way (apart from a bug in JDK's ArrayList's iterator impl) line
> 145 can throw an exception is when the iterator has no more elements, the
> test for that on line 113 suceeds and the iterator is reinitialized on line
> 142.
> 
> The code therefore assumes that it never gets an empty collection as a
> result of running a criteria query, which I think is wrong.

and

(Lukas Krejci comment #3)
> Well, an empty collection as a last page is not technically a wrong
> assumption, but it is no longer a correct assumption in the light of
> possibility of inconsistent results obtained from the criteria query.
> 
> The fact that you're only seeing this while deleting stuff en masse is an
> indicator that in fact that is what you might have uncovered.
Comment 1 Lukas Krejci 2013-10-02 08:15:41 EDT
I'm proposing a fix for this issue by merely loosening the assumptions around the total count of the results and moving the "refresh" logic from next() to hasNext().

Please review https://git.fedorahosted.org/cgit/rhq/rhq.git/commit/?id=9df66bb9643d8280fd83418738bbc07479630741
Comment 2 Lukas Krejci 2013-10-02 08:57:47 EDT
Could you please review the proposed fix?
Comment 3 Lukas Krejci 2013-10-02 08:58:13 EDT
Could you please review the proposed fix?
Comment 4 Jay Shaughnessy 2013-10-02 11:36:09 EDT
Lukas, this seems like a reasonable approach.  Good thinking.  It even cleans up the "delete" logic.

There is only one weakness I can think of, and that is a user that uses next() and looks for it to throw an Exception, as opposed to using hasNext().  It's very unlikely anyone will do that, although we could jdoc it to say you must use hasNext() prior to next().
Comment 5 Lukas Krejci 2013-10-02 11:40:53 EDT
Good point, Jay. Using just next() would give you NoSuchElementException much sooner than using the hasNext() + next() combo.

I'll update the implementation to accomomdate for that.
Comment 6 Lukas Krejci 2013-10-03 03:11:23 EDT
Not sure if there is something for QA to test here. This change is covered by unit tests but cannot be easily triggered from "outside". It was discovered while looking at BZ 1014300, which has been fixed so that it works even without this change.

The fix is important though, because CriteriaQuery.iterator() is the recommended (though not much used) way of iterating over the paged results inside the server codebase (it is NOT accessible remotely).

commit 74825e60875dc381da9dc452ab3b94b08d290f90
Author: Lukas Krejci <lkrejci@redhat.com>
Date:   Thu Oct 3 08:56:12 2013 +0200

    [BZ 1014524] - Fixing CriteriaQuery.iterator()
    
    The iterator implementation assumed the page list consistent with
    the page control, which might not be true, as brought to light by recent
    work on PageControl.isConsistentWith() method.
    
    The iterator implementation has been reworked to correctly iterate even
    over the inconsistent results by loosening the assumption on the total
    number of rows in all the pages.
Comment 7 Heiko W. Rupp 2014-04-23 08:31:59 EDT
Bulk closing of 4.10 issues.

If an issue is not solved for you, please open a new BZ (or clone the existing one) with a version designator of 4.10.

Note You need to log in before you can comment on or make changes to this bug.