Re: RFR 8168745 Iterator.forEachRemaining vs. Iterator.remove

[Paul Sandoz]

Stuart and I refined the text to mention a concurrent modification policy (e.g. 
fail-fast or weakly consistent) of any overriding class.

Paul.


diff -r c7b932897909 src/java.base/share/classes/java/lang/Iterable.java
--- a/src/java.base/share/classes/java/lang/Iterable.java   Wed Nov 23 
10:35:44 2016 -0800
+++ b/src/java.base/share/classes/java/lang/Iterable.java   Wed Nov 23 
10:40:53 2016 -0800
@@ -53,10 +53,13 @@
 /**
  * Performs the given action for each element of the {@code Iterable}
  * until all elements have been processed or the action throws an
- * exception.  Unless otherwise specified by the implementing class,
- * actions are performed in the order of iteration (if an iteration order
- * is specified).  Exceptions thrown by the action are relayed to the
+ * exception.  Actions are performed in the order of iteration, if that
+ * order is specified.  Exceptions thrown by the action are relayed to the
  * caller.
+ * 
+ * The behavior of this method is unspecified if the action performs
+ * side-effects that modify the underlying source of elements, unless an
+ * overriding class has specified a concurrent modification policy.
  *
  * @implSpec
  * The default implementation behaves as if:
diff -r c7b932897909 src/java.base/share/classes/java/util/Iterator.java
--- a/src/java.base/share/classes/java/util/Iterator.java   Wed Nov 23 
10:35:44 2016 -0800
+++ b/src/java.base/share/classes/java/util/Iterator.java   Wed Nov 23 
10:40:53 2016 -0800
@@ -76,10 +76,15 @@
 /**
  * Removes from the underlying collection the last element returned
  * by this iterator (optional operation).  This method can be called
- * only once per call to {@link #next}.  The behavior of an iterator
- * is unspecified if the underlying collection is modified while the
- * iteration is in progress in any way other than by calling this
- * method.
+ * only once per call to {@link #next}.
+ * 
+ * The behavior of an iterator is unspecified if the underlying collection
+ * is modified while the iteration is in progress in any way other than by
+ * calling this method, unless an overriding class has specified a
+ * concurrent modification policy.
+ * 
+ * The behavior of an iterator is unspecified if this method is called
+ * after a call to the {@link #forEachRemaining forEachRemaining} method.
  *
  * @implSpec
  * The default implementation throws an instance of
@@ -102,6 +107,13 @@
  * have been processed or the action throws an exception.  Actions are
  * performed in the order of iteration, if that order is specified.
  * Exceptions thrown by the action are relayed to the caller.
+ * 
+ * The behavior of an iterator is unspecified if the action modifies the
+ * collection in any way (even by calling the {@link #remove remove} 
method),
+ * unless an overriding class has specified a concurrent modification 
policy.
+ * 
+ * Subsequent behavior of an iterator is unspecified if the action throws 
an
+ * exception.
  *
  * @implSpec
  * The default implementation behaves as if:

Re: RFR 8168745 Iterator.forEachRemaining vs. Iterator.remove

[Martin Buchholz]

On Mon, Nov 21, 2016 at 2:29 PM, Paul Sandoz  wrote:

>
> A linked list based implementation could report hasNext() == false when
> currentNode.next == null, but that might change before the next call to
> hasNext().  AFAIK, all of our concrete implementations don't have this
> property - the iterator being exhausted is a "sticky" property, in part
> because it's nice to the GC to null out fields at the end.  One can imagine
> an iterator (or spliterator) designed to work differently, e.g. tryAdvance
> would be a way of "polling" the stream of elements to see whether any are
> available, and would eventually visit all elements added.
>
>
> Back in the real world, forEachRemaining is a particularly strong hint
> that this iterator/spliterator should be forever exhausted.
>
>
> Yes :-) i would argue as strong a hint as that of hasNext returning false.
>
>
Stronger, perhaps!  There's a corner case where a user sees hasNext()
returning false, then calls remove().  Perhaps it's a weird way of deleting
the last element. That seems more defensible user code than calling
remove() after forEachRemaining.  In some classes maintaining the ability
to delete the last element is a burden, as in ArrayBlockingQueue.


> Paul.
>
> Perhaps where that matters (ArrayBlockingQueue) we could even add such a
> thing to the spec.  We discourage use of Iterators with ABQ, in part
> because in the past there was no way to "close" an Iterator.  But now there
> is!
>
>
>

Re: RFR 8168745 Iterator.forEachRemaining vs. Iterator.remove

[Paul Sandoz]

> On 21 Nov 2016, at 13:21, Martin Buchholz  wrote:
> 
> 
> 
> On Mon, Nov 21, 2016 at 1:01 PM, Paul Sandoz  > wrote:
> 
> > There's the question of what to promise after a call to forEachRemaining 
> > (whether or not an action threw).  Perhaps a blanket
> > """Subsequent behavior of an iterator is unspecified after a call to this 
> > method."""
> >
> 
> I think that is overly limiting, it’s exceptions and remove that are 
> problematic. Assuming no exception it is fine to subsequently call next, 
> hasNext and forEachRemaining, each has well defined behaviour (throws 
> NoSuchElementException, false, no-op respectively).
> 
> In a concurrent world, it's not so clear.

Good point, yes, i was thinking non-concurrent, but i would still expect it to 
hold for weakly consistent iterators.


> A linked list based implementation could report hasNext() == false when 
> currentNode.next == null, but that might change before the next call to 
> hasNext().  AFAIK, all of our concrete implementations don't have this 
> property - the iterator being exhausted is a "sticky" property, in part 
> because it's nice to the GC to null out fields at the end.  One can imagine 
> an iterator (or spliterator) designed to work differently, e.g. tryAdvance 
> would be a way of "polling" the stream of elements to see whether any are 
> available, and would eventually visit all elements added.
> 
> Back in the real world, forEachRemaining is a particularly strong hint that 
> this iterator/spliterator should be forever exhausted.

Yes :-) i would argue as strong a hint as that of hasNext returning false.

Paul.

> Perhaps where that matters (ArrayBlockingQueue) we could even add such a 
> thing to the spec.  We discourage use of Iterators with ABQ, in part because 
> in the past there was no way to "close" an Iterator.  But now there is!

Re: RFR 8168745 Iterator.forEachRemaining vs. Iterator.remove

[Martin Buchholz]

On Mon, Nov 21, 2016 at 1:01 PM, Paul Sandoz  wrote:

>
> > There's the question of what to promise after a call to forEachRemaining
> (whether or not an action threw).  Perhaps a blanket
> > """Subsequent behavior of an iterator is unspecified after a call to
> this method."""
> >
>
> I think that is overly limiting, it’s exceptions and remove that are
> problematic. Assuming no exception it is fine to subsequently call next,
> hasNext and forEachRemaining, each has well defined behaviour (throws
> NoSuchElementException, false, no-op respectively).
>

In a concurrent world, it's not so clear.  A linked list based
implementation could report hasNext() == false when currentNode.next ==
null, but that might change before the next call to hasNext().  AFAIK, all
of our concrete implementations don't have this property - the iterator
being exhausted is a "sticky" property, in part because it's nice to the GC
to null out fields at the end.  One can imagine an iterator (or
spliterator) designed to work differently, e.g. tryAdvance would be a way
of "polling" the stream of elements to see whether any are available, and
would eventually visit all elements added.

Back in the real world, forEachRemaining is a particularly strong hint that
this iterator/spliterator should be forever exhausted.  Perhaps where that
matters (ArrayBlockingQueue) we could even add such a thing to the spec.
We discourage use of Iterators with ABQ, in part because in the past there
was no way to "close" an Iterator.  But now there is!

Re: RFR 8168745 Iterator.forEachRemaining vs. Iterator.remove

[Paul Sandoz]

> On 21 Nov 2016, at 12:32, Martin Buchholz  wrote:
> 
> Thanks.  Seems like progress.
> 
> The spec below seems like it could be improved, but not sure how...
> 
> + * 
> + * The behavior of an iterator is unspecified if the action performs the
> + * following side-effects:
> + * 
> + * modifies the underlying collection; or
> + * calls the {@link #remove remove} method.
> + * 
> + * 
> + * Subsequent behavior of an iterator is unspecified if the action 
> throws an
> + * exception.
> 
> 
> Maybe """The behavior of an iterator is unspecified if an action modifies the 
> collection in any way (even by calling the {@link #remove remove} method).”””

I am ok with that.


> There's the question of what to promise after a call to forEachRemaining 
> (whether or not an action threw).  Perhaps a blanket
> """Subsequent behavior of an iterator is unspecified after a call to this 
> method."""
> 

I think that is overly limiting, it’s exceptions and remove that are 
problematic. Assuming no exception it is fine to subsequently call next, 
hasNext and forEachRemaining, each has well defined behaviour (throws 
NoSuchElementException, false, no-op respectively).


> The default implementation cannot ensure desirable properties we might easily 
> implement for a concrete implementation, e.g. whether the iterator is 
> exhausted after a call to forEachRemaining where an action threw.
> 

It does tie our hands somewhat.


> Should we be aligning Iterator and Spliterator, which have similar questions?

Thankfully Spliterator has no remove and no default methods :-)  so we are in 
an easier position, but yes we could align regarding exceptions thrown by 
actions.

Paul.

Re: RFR 8168745 Iterator.forEachRemaining vs. Iterator.remove

[Martin Buchholz]

Thanks.  Seems like progress.

The spec below seems like it could be improved, but not sure how...

+ * 
+ * The behavior of an iterator is unspecified if the action performs
the
+ * following side-effects:
+ * 
+ * modifies the underlying collection; or
+ * calls the {@link #remove remove} method.
+ * 
+ * 
+ * Subsequent behavior of an iterator is unspecified if the action
throws an
+ * exception.


Maybe """The behavior of an iterator is unspecified if an action modifies
the collection in any way (even by calling the {@link #remove remove}
method)."""

There's the question of what to promise after a call to forEachRemaining
(whether or not an action threw).  Perhaps a blanket
"""Subsequent behavior of an iterator is unspecified after a call to this
method."""

The default implementation cannot ensure desirable properties we might
easily implement for a concrete implementation, e.g. whether the iterator
is exhausted after a call to forEachRemaining where an action threw.

Should we be aligning Iterator and Spliterator, which have similar
questions?

RFR 8168745 Iterator.forEachRemaining vs. Iterator.remove

[Paul Sandoz]

Hi,

Please review these specification clarifications to the methods 
Iterator.forEachRemaining and Iterator.remove.

Implementations of Iterator.forEachRemaining should have some wiggle room to 
optimize traversal. (In hindsight we could have done a better job locking such 
behaviour down in Java 8.)

I also took the opportunity to update the Iterable.forEach method.

Thanks,
Paul.

diff -r 4bf7aaa0d611 src/java.base/share/classes/java/lang/Iterable.java
--- a/src/java.base/share/classes/java/lang/Iterable.java   Thu Nov 17 
12:24:51 2016 -0800
+++ b/src/java.base/share/classes/java/lang/Iterable.java   Mon Nov 21 
10:55:21 2016 -0800
@@ -53,10 +53,12 @@
 /**
  * Performs the given action for each element of the {@code Iterable}
  * until all elements have been processed or the action throws an
- * exception.  Unless otherwise specified by the implementing class,
- * actions are performed in the order of iteration (if an iteration order
- * is specified).  Exceptions thrown by the action are relayed to the
+ * exception.  Actions are performed in the order of iteration, if that
+ * order is specified.  Exceptions thrown by the action are relayed to the
  * caller.
+ * 
+ * The behaviour of this method is unspecified if the action performs
+ * side-effects that modify the underlying source of elements.
  *
  * @implSpec
  * The default implementation behaves as if:
diff -r 4bf7aaa0d611 src/java.base/share/classes/java/util/Iterator.java
--- a/src/java.base/share/classes/java/util/Iterator.java   Thu Nov 17 
12:24:51 2016 -0800
+++ b/src/java.base/share/classes/java/util/Iterator.java   Mon Nov 21 
10:55:21 2016 -0800
@@ -76,10 +76,15 @@
 /**
  * Removes from the underlying collection the last element returned
  * by this iterator (optional operation).  This method can be called
- * only once per call to {@link #next}.  The behavior of an iterator
- * is unspecified if the underlying collection is modified while the
- * iteration is in progress in any way other than by calling this
- * method.
+ * only once per call to {@link #next}.
+ * 
+ * The behavior of an iterator is unspecified if:
+ * 
+ * the underlying collection is modified while the iteration is in
+ * progress in any way other than by calling this method; or
+ * this method is called after a call to the
+ * {@link #forEachRemaining forEachRemaining} method.
+ * 
  *
  * @implSpec
  * The default implementation throws an instance of
@@ -102,6 +107,16 @@
  * have been processed or the action throws an exception.  Actions are
  * performed in the order of iteration, if that order is specified.
  * Exceptions thrown by the action are relayed to the caller.
+ * 
+ * The behavior of an iterator is unspecified if the action performs the
+ * following side-effects:
+ * 
+ * modifies the underlying collection; or
+ * calls the {@link #remove remove} method.
+ * 
+ * 
+ * Subsequent behavior of an iterator is unspecified if the action throws 
an
+ * exception.
  *
  * @implSpec
  * The default implementation behaves as if: