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

David Holmes david.holmes at oracle.com
Fri Mar 9 09:53:42 UTC 2012

Hi Sean,

That seems to implement the required semantics.

Minor style nit: }else{ -> } else {

Not sure about the testcase ... Can size() not remove some elements directly but return the original size?

David

On 9/03/2012 6:16 PM, Sean Chou wrote:

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.testconcurrentRemove(); } public void testconcurrentRemove() { 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

More information about the core-libs-dev mailing list