Re: [patch] Re: HashMap putAll/putAllInternal bug

[Stuart Ballard]

Brian Jones wrote:
> Cool, don't want to hold you up on that too much.  Wondering how Sun's
> impl makes a copy of the given Collection while avoiding copying an
> object being modified in another thread.  Anyone know?  I think the
> Iterators are supposed to throw exceptions when this occurs too.

I don't think this is affected too much either way by my patch. If 
another thread is actively modifying the collection being added at the 
time we're trying to add it, and it's not a thread-safe collection, the 
results are undefined. If it *is* a thread-safe collection, the most 
that's guaranteed is that you'll get a ConcurrentModificationException 
in the right place.

(Actually, my patch probably helps with that case too: by only 
incrementing this.size as each item is actually added, the state of the 
Map is closer to valid if a ConcurrentModificationException is thrown 
part-way through the iteration. Perhaps it could be tweaked to be even 
better in that regard by putting the "size++" *after* the call to next()).

My patch really only affects what happens if the value returned by 
size() is incorrect for a particular Map. Without the patch, Classpath 
blindly believes size() and attempts to add exactly that many elements, 
throwing an exception if there are actually less and not adding all of 
them if there are actually more. With the patch, it relies on the 
iterator to determine how many items there actually are.

I have a map where it's prohibitively expensive[1] to calculate the 
correct value of size(). It would be possible, but a LOT of 
collections-using code assumes that size() is fast - the very code that 
this patch changes is an example, where it was assumed that calling 
size() was much faster than calling hasNext() for each element and 
therefore treated as an optimization. So the best I could do was to 
cache a "best guess" at the size and return that from size(), and have 
iterator() give the canonical information. On Sun's JDK, or on Classpath 
with the patch, this works. Without it, it doesn't.

Stuart.

[1] A correct implementation of size() for my map would actually look 
something like this:

public int size() {
  int size = 0;
  for (Iterator i = iterator(); i.hasNext(); ) {
    i.next();
    size++;
  }
  return size;
}

--
Stuart Ballard, Senior Web Developer
FASTNET - Web Solutions
(215) 283-2300, ext. 126
www.fast.net