On 14/12/2011 10:27 PM, Sean Chou wrote:
Thank you as well~ I'll work on it.
I'm not sure it is worth fixing by changing the code. A change to the
implementation note might be simpler and better.
Although it is easy to detect when this behavioural mismatch occurs, I
can not conceive a meaningful situation where this would make a
difference. The code would have to assume the passed in array is large
enough and somehow know that even if the collection grows during the
call that it will shrink again - that seems unknowable to me.
David
-----
On Wed, Dec 14, 2011 at 4:02 PM, David Holmes <david.hol...@oracle.com
<mailto:david.hol...@oracle.com>> wrote:
On 14/12/2011 5:05 PM, Sean Chou wrote:
Hi Mike, David,
I reported this as a bug:
http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=7121314
<http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7121314> .
Thanks - I've turned the initial incident report into a "real" bug
report.
David
On Wed, Dec 14, 2011 at 1:03 PM, Mike Duigou
<mike.dui...@oracle.com <mailto:mike.dui...@oracle.com>
<mailto:mike.dui...@oracle.com
<mailto:mike.dui...@oracle.com>__>> wrote:
I agree that most people probably won't check whether the array
returned is the array they provided. It is possible that they
incorrectly assume it is. ie.
Collection<Integer> collection;
...
Integer[] foo = new Integer[collection.size()]; // of sufficient
size for all elements
// at this point collection grows.
collection.toArray(foo); // this will work, right?
for(Integer each : foo) {
// wait! foo is empty, why?
}
If collection is mutated and increased in size between the
creation
of foo and the toArray operation then foo will be empty and
the user
probably won't know why the elements weren't copied.
Actually, the
elements were copied but the result of toArray containing those
elements was ignored.
This usage seems like just a bad idiom though and is avoidable.
More worrying is the toArray() case where the resultant array is
currently documented to hold only result elements and no extra
nulls. If the collection shrinks then it is currently
possible that
the resulting array could very unexpectedly hold nulls.
Mike
On Dec 13 2011, at 05:30 , Sean Chou wrote:
> Sorry for the confuse. By "ok", I mean "compare the size of array
which is
> going to be
> returned and the size of the specified array, and copy the
elements
> into the specified
> array if it is larger and return the specified array."
>
> Nothing is causing problem for now, I just found a mismatch. I
think most
> guys will
> just use the returned array without checking if it's the
specified one; and
> this is also
> why I think it may be possible to modify the behavior without
causing
> problems.
>
> And I think modifying ConcurrentHashMap is as dangerous as
modifying
> AbstractCollection
> if people are relying on implementation, is this right? So it
seems we can
> do nothing
> to the mismatch now...
>
>
> On Tue, Dec 13, 2011 at 8:06 PM, David Holmes
<david.hol...@oracle.com <mailto:david.hol...@oracle.com>
<mailto:david.holmes@oracle.__com
<mailto:david.hol...@oracle.com>>>wrote:
>
>> On 13/12/2011 9:18 PM, Sean Chou wrote:
>>
>>> Hi ,
>>>
>>> Is it possible to change the spec ? I found it is defined in
>>> java.utils.Collection interface. It would be easy for
>>> AbstractCollection to state that it is not designed for
concurrent
>>> operations, and its subclass should take care of them.
>>>
>>
>> Such a disclaimer might be added to the spec for
AbstractCollection but
>> that doesn't really change anything - it just makes observed
behaviour less
>> surprising.
>>
>>
>> However, I think the simplest way may be modifying
toArray(T[])
>>> method for an additional check, which would work for most
subclasses of
>>> AbstractCollection...
>>> Is that ok ?
>>>
>>
>> "ok" in what sense? Do you want to change the spec or just
change the
>> current behaviour? If you do the latter and people rely on that
>> implementation rather than the spec then code will not be
portable across
>> different implementations of the platform.
>>
>> I would not want to see a change in behaviour without a
change in
>> specification and I do not think the specification for
AbstractCollection
>> can, or should be, changed. Just my opinion of course.
>>
>> What is the concrete concurrent collection that you have a
problem with?
>> If it is ConcurrentHashMap, as per the example, then perhaps
>> ConcurrentHashMap should be where a change is considered.
>>
>> David
>> -----
>>
>>
>>> On Tue, Dec 13, 2011 at 3:41 PM, David Holmes
<david.hol...@oracle.com <mailto:david.hol...@oracle.com>
<mailto:david.holmes@oracle.__com <mailto:david.hol...@oracle.com>>
>>> <mailto:david.holmes@oracle <mailto:david.holmes@oracle>.
<mailto:david.holmes@oracle <mailto:david.holmes@oracle>.>*__*com
<david.hol...@oracle.com <mailto:david.hol...@oracle.com>
<mailto:david.holmes@oracle.__com
<mailto:david.hol...@oracle.com>>>>> wrote:
>>>
>>> Hi Sean,
>>>
>>>
>>> On 13/12/2011 5:21 PM, Sean Chou wrote:
>>>
>>> When I was reading the code of
AbstractCollection.toArray(T[] ), I
>>> found its behavior maybe different from the spec in
multithread
>>> environment. The spec says "If the collection fits in the
specified
>>> array, it is returned therein. Otherwise, a new array is
allocated
>>> with the runtime type of the specified array and the size
of this
>>> collection." However, in multithread environment, it is
not easy
>>> to
>>> tell if the collection fits in the specified array,
because the
>>> items may be removed when toArray is copying.
>>>
>>>
>>> Right. The problem is that AbstractCollection doesn't address
>>> thread-safety or any other concurrency issues so doesn't
account for
>>> the collection growing or shrinking while the toArray
snapshot is
>>> being taken. Really the collection implementations that are
designed
>>> to support multiple threads should override toArray to make
it clear
>>> how it should behave. As it stands, in my opinion, it is
more a
>>> "quality of implementation" issue as to whether
AbstractCollection
>>> expends effort after creating the array to see if the
array is
>>> actually full or not; or whether after creating an array
it turns
>>> out it could have fit in the original.
>>>
>>> For a concurrent collection I would write the spec for
toArray
>>> something like:
>>>
>>> "The current size of the collection is examined and if the
>>> collection fits in the specified array it will be the target
array,
>>> else a new array is allocated based on that current size
and it
>>> becomes the target array. If the collection grows such
that the
>>> target array no longer fits then extra elements will not be
copied
>>> into the target array. If the collection shrinks then the
target
>>> array will contain null elements."
>>>
>>> Or for the last part "then the target array will be copied to
a new
>>> array that exactly fits the number of elements returned".
>>>
>>> David Holmes
>>> ------------
>>>
>>>
>>>
>>> Here is a testcase:
>>>
//////////////////////////////__**__//////////////////////////__//**
>>> //__//////////////
>>>
>>> import java.util.Map;
>>> import java.util.concurrent.__**__ConcurrentHashMap;
>>>
>>>
>>> public class CollectionToArrayTest {
>>>
>>> static volatile Map<String, String> map = new
>>> TConcurrentHashMap<String, String>();
>>> static volatile boolean gosleep = true;
>>>
>>> static class TConcurrentHashMap<K, V> extends
>>> ConcurrentHashMap<K, V> {
>>> public int size() {
>>> int oldresult = super.size();
>>> System.out.println("map size before
concurrent
>>> remove is "
>>> + oldresult);
>>> while (gosleep) {
>>> try {
>>> // Make sure the map is modified
during
>>> toArray is
>>> called,
>>> // between getsize and being
iterated.
>>> Thread.sleep(1000);
>>> // System.out.println("size called,
size is "
>>> +
>>> oldresult +
>>> // " take a sleep to make sure the
element
>>> is deleted
>>> before size is returned.");
>>> } catch (Exception e) {
>>> }
>>> }
>>> return oldresult;
>>> }
>>> }
>>>
>>> static class ToArrayThread implements Runnable {
>>> public void run() {
>>> for (int i = 0; i< 5; i++) {
>>> String str = Integer.toString(i);
>>> map.put(str, str);
>>> }
>>> String[] buffer = new String[4];
>>> String[] strings =
map.values().toArray(buffer);
>>> // System.out.println("length is " +
strings.length);
>>> if (strings.length<= buffer.length) {
>>> System.out.println("given array size
is "
>>> + buffer.length
>>> + " \nreturned array
size is "
>>> + strings.length
>>> + ", \nbuffer should
be used
>>> according to
>>> spec. Is buffer used : "
>>> + (strings == buffer));
>>> }
>>> }
>>> }
>>>
>>> static class RemoveThread implements Runnable {
>>> public void run() {
>>> String str = Integer.toString(0);
>>> map.remove(str);
>>> gosleep = false;
>>> }
>>> }
>>>
>>> public static void main(String args[]) {
>>> CollectionToArrayTest app = new
CollectionToArrayTest();
>>> app.test_concurrentRemove();
>>> }
>>>
>>> public void test_concurrentRemove() {
>>>
>>>
System.out.println("//////////__**__//////////////////////////__//**
>>> //__//////\n"
>>>
>>> +
>>> "The spec says if the given array is large\n " +
>>> "enough to hold all elements, the given array\n" +
>>> "should be returned by toArray. This \n" +
>>> "testcase checks this case. \n" +
>>> "/////////////////////////////__**__/////////////////");
>>>
>>>
>>> Thread[] threads = new Thread[2];
>>> threads[0] = new Thread(new ToArrayThread());
>>> threads[1] = new Thread(new RemoveThread());
>>>
>>> threads[0].start();
>>>
>>> try {
>>> // Take a sleep to make sure toArray is
already
>>> called.
>>> Thread.sleep(1200);
>>> } catch (Exception e) {
>>> }
>>>
>>> threads[1].start();
>>> }
>>> }
>>>
>>>
//////////////////////////////__**__//////////////////////////__//**
>>> //__//////////////
>>>
>>>
>>> TConcurrentHashMap is used to make sure the collection is
modified
>>> during toArray is invoked. So the returned array fits
in the
>>> specified
>>> array, but a new array is used because toArray checks the
size
>>> before copying.
>>>
>>> Is this a bug ?
>>>
>>>
>>>
>>>
>>>
>>> --
>>> Best Regards,
>>> Sean Chou
>>>
>>>
>
>
> --
> Best Regards,
> Sean Chou
--
Best Regards,
Sean Chou
--
Best Regards,
Sean Chou
