Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec

[Sean Chou]

Hi all,

    AbstractCollection.toArray(T[] ) might return a new array even if the
given array has enough room for the returned elements when it is
concurrently modified. This behavior violates the spec documented in
java.util.Collection .
    This patch checks the size of returned array and copies the elements to
return to the given array if it is large enough.

    The webrev is at :
http://cr.openjdk.java.net/~zhouyx/7121314/webrev.00/

    The discussion about this bug happened several months ago, the link to
the start of that discussion is:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/008686.html

    And people in that discussion are also in cc list. David mentioned it
might be simple and better to change the implementation note in last mail
of that discussion, however the code change is simple too.

    There is a test case in the previous discussion. It is not included in
the webrev, because the testcase is heavily implementation dependent. I
will add it if it is requested.

Also paste the testcase here:

////////////////////////////////////////////////////////////

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();
    }
}

////////////////////////////////////////////////


-- 
Best Regards,
Sean Chou