Bug 1014524

Summary:	CriteriaQuery Iterator #next() throws java.util.NoSuchElementException
Product:	[Other] RHQ Project	Reporter:	Thomas Segismont <tsegismo>
Component:	Core Server	Assignee:	RHQ Project Maintainer <rhq-maint>
Status:	CLOSED CURRENTRELEASE	QA Contact:	Mike Foley <mfoley>
Severity:	high	Docs Contact:
Priority:	medium
Version:	4.9	CC:	hrupp, jshaughn, lkrejci, spinder
Target Milestone:	---
Target Release:	RHQ 4.10
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2014-04-23 12:31:59 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Thomas Segismont 2013-10-02 09:35:32 UTC

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 12:15:41 UTC

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 12:57:47 UTC

Could you please review the proposed fix?

Comment 3 Lukas Krejci 2013-10-02 12:58:13 UTC

Could you please review the proposed fix?

Comment 4 Jay Shaughnessy 2013-10-02 15:36:09 UTC

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 15:40:53 UTC

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 07:11:23 UTC

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>
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 12:31:59 UTC

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.