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

[David Holmes]

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.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
>