"for each" causing exception where an iterator did not in 1.5 ...

  • Thread starter Charles Morison
  • Start date
C

Charles Morison

An explanation of why this happens would be great. To wit:

Given a collection defined as follows:

private java.util.List<TableEntryStatusNode> tableEntryStatusNodes;

And constructed as follows (it is used in a multi-threaded environment):

tableEntryStatusNodes = Collections.synchronizedList (new
LinkedList<TableEntryStatusNode> ());

I used to be able to scan the collection to remove elements from the
collection using the iterator approach as follows with out any problems:

for (Iterator<TableEntryStatusNode> iterator =
tableEntryStatusNodes.listIterator (); iterator.hasNext (); )
{
if (iterator.next ().getBasicType () ==
TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
iterator.remove ();
}
}

I attempted to change the code to the new "for each" approach provided
in 1.5 as follows:

for (TableEntryStatusNode node : tableEntryStatusNodes)
{
if (node.getBasicType () == TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
tableEntryStatusNodes.remove (node);
}
}

This resulted in a run-time error with the tail end of the stack trace
stating:

Exception in thread "AWT-EventQueue-0"
java.util.ConcurrentModificationException
at java.util.LinkedList$ListItr.checkForComodification(Unknown Source)
at java.util.LinkedList$ListItr.next(Unknown Source)

This left me with the need for two for loops in order to do what one for
loop did before if I wanted to use the new "for each" approach:

java.util.List<TableEntryStatusNode> nodesToRemove = new
LinkedList<TableEntryStatusNode> ();
for (TableEntryStatusNode node : tableEntryStatusNodes)
{
if (node.getBasicType () == TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
nodesToRemove.add (node);
}
}

if (!nodesToRemove.isEmpty ())
{
for (TableEntryStatusNode node : nodesToRemove)
{
tableEntryStatusNodes.remove (node);
}
}

So it would seem that the "for each" construct is not a completely
transparent replacement for using iterators.

Is this mentioned by Sun or is this an error in their implementation?
Or is it an error in my implementation?

Thanks for any help in advance.
 
A

Andrew McDonagh

Charles said:
An explanation of why this happens would be great. To wit:

Given a collection defined as follows:

private java.util.List<TableEntryStatusNode> tableEntryStatusNodes;

And constructed as follows (it is used in a multi-threaded environment):

tableEntryStatusNodes = Collections.synchronizedList (new
LinkedList<TableEntryStatusNode> ());

I used to be able to scan the collection to remove elements from the
collection using the iterator approach as follows with out any problems:

for (Iterator<TableEntryStatusNode> iterator =
tableEntryStatusNodes.listIterator (); iterator.hasNext (); )
{
if (iterator.next ().getBasicType () ==
TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
iterator.remove ();
}
}

I attempted to change the code to the new "for each" approach provided
in 1.5 as follows:

for (TableEntryStatusNode node : tableEntryStatusNodes)
{
if (node.getBasicType () == TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
tableEntryStatusNodes.remove (node);
}
}

This resulted in a run-time error with the tail end of the stack trace
stating:

Exception in thread "AWT-EventQueue-0"
java.util.ConcurrentModificationException
at java.util.LinkedList$ListItr.checkForComodification(Unknown Source)
at java.util.LinkedList$ListItr.next(Unknown Source)

This left me with the need for two for loops in order to do what one for
loop did before if I wanted to use the new "for each" approach:

java.util.List<TableEntryStatusNode> nodesToRemove = new
LinkedList<TableEntryStatusNode> ();
for (TableEntryStatusNode node : tableEntryStatusNodes)
{
if (node.getBasicType () == TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
nodesToRemove.add (node);
}
}

if (!nodesToRemove.isEmpty ())
{
for (TableEntryStatusNode node : nodesToRemove)
{
tableEntryStatusNodes.remove (node);
}
}

So it would seem that the "for each" construct is not a completely
transparent replacement for using iterators.

Is this mentioned by Sun or is this an error in their implementation? Or
is it an error in my implementation?

Thanks for any help in advance.

Haven't done a great deal with the new 1.5 stuff yet, so this may be
totally off, but it looks like your old code was calling

iterator.remove();

Which is prevents the concurrentModificationExceptions, because the
Iterator is kept in sync with the change.

However, your new code is in reality calling

List.remove(object);

whilst your iterators are iterating, therefore resulting in the exception.

The new For construct doesn't give you the iterator, it give you the
object at the iterators position.
 
X

xarax

Charles Morison said:
An explanation of why this happens would be great. To wit:

Given a collection defined as follows:

private java.util.List<TableEntryStatusNode> tableEntryStatusNodes;

And constructed as follows (it is used in a multi-threaded environment):

tableEntryStatusNodes = Collections.synchronizedList (new
LinkedList<TableEntryStatusNode> ());

I used to be able to scan the collection to remove elements from the
collection using the iterator approach as follows with out any problems:

for (Iterator<TableEntryStatusNode> iterator =
tableEntryStatusNodes.listIterator (); iterator.hasNext (); )
{
if (iterator.next ().getBasicType () ==
TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
iterator.remove ();
}
}

I attempted to change the code to the new "for each" approach provided
in 1.5 as follows:

for (TableEntryStatusNode node : tableEntryStatusNodes)
{
if (node.getBasicType () == TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
tableEntryStatusNodes.remove (node);
}
}

This resulted in a run-time error with the tail end of the stack trace
stating:

Exception in thread "AWT-EventQueue-0"
java.util.ConcurrentModificationException
at java.util.LinkedList$ListItr.checkForComodification(Unknown Source)
at java.util.LinkedList$ListItr.next(Unknown Source)

This left me with the need for two for loops in order to do what one for
loop did before if I wanted to use the new "for each" approach:

java.util.List<TableEntryStatusNode> nodesToRemove = new
LinkedList<TableEntryStatusNode> ();
for (TableEntryStatusNode node : tableEntryStatusNodes)
{
if (node.getBasicType () == TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
nodesToRemove.add (node);
}
}

if (!nodesToRemove.isEmpty ())
{
for (TableEntryStatusNode node : nodesToRemove)
{
tableEntryStatusNodes.remove (node);
}
}

So it would seem that the "for each" construct is not a completely
transparent replacement for using iterators.

Is this mentioned by Sun or is this an error in their implementation?
Or is it an error in my implementation?

Thanks for any help in advance.

It appears to be an error in your interpretation
of when to use enhanced "for each" loop. The compiler
hides the extraction and usage of the iterator instance,
and it only uses the "hasNext()" and "next()" methods
of the iterator. The "remove()" method is unavailable.
So, you cannot use the enhanced "for loop" when you
need to remove elements from the collection.
 
C

Charles Morison

Umm, I don't think you read the exception correctly, I am using the
remove method on the linked list collection itself, not on the object
found in the collection that meets the criteria for removal. My point
is that although in everything I've read about the "for each" loop on
Sun's Java website they promote it as an improvement on using iterators.
I am showing a case in which it is not.
 
C

Charles Morison

Sorry, I now understand what you are saying. I agree that I don't have
access to the iterator's remove method (because I don't have access to
the iterator), but I disagree that it isn't available as it is part of
the Iterator interface. In the future it might be nice if Java would
allow read-only access to the iterator that is being used behind the
scenes in the "for each" loop (given the exception thrown it is obvious
that the compiler is till using a list iterator to do what the "for
each" loop does). That way the user can delete objects from the
collection synchronously.
 
C

Charles Morison

Guess I should have read the javadoc for a list iterator:

The list-iterator is fail-fast: if the list is structurally modified at
any time after the Iterator is created, in any way except through the
list-iterator's own remove or add methods, the list-iterator will throw
a ConcurrentModificationException. Thus, in the face of concurrent
modification, the iterator fails quickly and cleanly, rather than
risking arbitrary, non-deterministic behavior at an undetermined time in
the future.

Still frustrating that Java promotes these new syntax features seemingly
without thinking them through and trying to determine what operations
are effected (such as adding or removing objects from a collection).

Oh well, thanks for the help.
 
T

Tony Morris

Charles Morison said:
An explanation of why this happens would be great. To wit:

Given a collection defined as follows:

private java.util.List<TableEntryStatusNode> tableEntryStatusNodes;

And constructed as follows (it is used in a multi-threaded environment):

tableEntryStatusNodes = Collections.synchronizedList (new
LinkedList<TableEntryStatusNode> ());

I used to be able to scan the collection to remove elements from the
collection using the iterator approach as follows with out any problems:

for (Iterator<TableEntryStatusNode> iterator =
tableEntryStatusNodes.listIterator (); iterator.hasNext (); )
{
if (iterator.next ().getBasicType () ==
TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
iterator.remove ();
}
}

I attempted to change the code to the new "for each" approach provided
in 1.5 as follows:

for (TableEntryStatusNode node : tableEntryStatusNodes)
{
if (node.getBasicType () == TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
tableEntryStatusNodes.remove (node);
}
}

This resulted in a run-time error with the tail end of the stack trace
stating:

Exception in thread "AWT-EventQueue-0"
java.util.ConcurrentModificationException
at java.util.LinkedList$ListItr.checkForComodification(Unknown Source)
at java.util.LinkedList$ListItr.next(Unknown Source)

This left me with the need for two for loops in order to do what one for
loop did before if I wanted to use the new "for each" approach:

java.util.List<TableEntryStatusNode> nodesToRemove = new
LinkedList<TableEntryStatusNode> ();
for (TableEntryStatusNode node : tableEntryStatusNodes)
{
if (node.getBasicType () == TableEntryStatusNode.BASIC_TYPE_NOT_USED)
{
nodesToRemove.add (node);
}
}

if (!nodesToRemove.isEmpty ())
{
for (TableEntryStatusNode node : nodesToRemove)
{
tableEntryStatusNodes.remove (node);
}
}

So it would seem that the "for each" construct is not a completely
transparent replacement for using iterators.

Is this mentioned by Sun or is this an error in their implementation?
Or is it an error in my implementation?

Thanks for any help in advance.


Nothing to do with the foreach loop; everything to do with how you are
concurrently modifying a Collection during iteration.
The foreach loop compiles to bytecode similar to what your original code
does (some important differences, but irrelevant to your problem).
 
T

Tony Morris

I am showing a case in which it is not.

You aren't.
The intended uses of foreach are well documented in the JSR and also in the
language documentation.
You do not have access to the Iterator<E> instance that the compiler inserts
or the current count in the loop (if you find yourself needing one, use a
traditional for loop over inserting one yourself into a foreach) - only the
Iterable<E> and the element being iterated.
You have shown one of many ways in which the foreach is not intended to be
used.

After just completing a 15KLOC software component using every 1.5 language
feature, I'd have to agree with Sun, foreach is an improvement.
In fact, my personal opinion is that every language feature has some benefit
when applied correctly, with the one exception of autoboxing/boxing, which
introduces the potential for error without a compile-time check all in the
name of "less typing" (Java is a verbose language anyway).
The foreach does have documented limitations however and you have
discoevered one of them for yourself. "The car is a silly invention - it
doesn't fly.", wrong tool, wrong job.
 
A

Andrew McDonagh

Tony said:
After just completing a 15KLOC software component using every 1.5 language
feature, I'd have to agree with Sun, foreach is an improvement.
In fact, my personal opinion is that every language feature has some benefit
when applied correctly, with the one exception of autoboxing/boxing, which
introduces the potential for error without a compile-time check all in the
name of "less typing" (Java is a verbose language anyway).
The foreach does have documented limitations however and you have
discoevered one of them for yourself. "The car is a silly invention - it
doesn't fly.", wrong tool, wrong job.

Just curious as to what you and others felt is wrong with
autoboxing/boxing. I've yet to use 1.5 in anger and so any insights you
have gained since using it would be greatly appreciated.
 
A

Andrew McDonagh

Tony said:
Nothing to do with the foreach loop; everything to do with how you are
concurrently modifying a Collection during iteration.
The foreach loop compiles to bytecode similar to what your original code
does (some important differences, but irrelevant to your problem).

You seem to have missed his initial code, it was performing:
iterator.remove();

Then the OP changed the for loop to use the new foreach construct,
misinterpreting its usage. So now his code was actually performing:
list.remove(object), which as you know will fail fast.
 
S

Stefan Schulz

Just curious as to what you and others felt is wrong with
autoboxing/boxing. I've yet to use 1.5 in anger and so any insights
you have gained since using it would be greatly appreciated.

Three gripes:

* It punches a hole in the otherwise extremely tight type
systen java employs. Primitives are not objects in this language, and
pretending they were opens up a lot of potential for mistakes

* Incomplete support. You still can not write 1.hashCode(). (Not that
you usually would, but if they really were objects, you should be able
to).

* It encourages bad programming practices. Autoboxing litters the heap
with thousands of short-lived wrapper objects, which the programmer
often neither sees nor expects.
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

Forum statistics

Threads
474,436
Messages
2,571,696
Members
48,796
Latest member
Greg L.
Top