Request for review : 7121314 : Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec
Charles Lee
littlee at linux.vnet.ibm.com
Tue Apr 10 06:03:54 UTC 2012
Hi Sean,
The the patch has been committed @
Changeset: e06ea0dd9207
Author: littlee
Date: 2012-04-10 10:17 +0800
URL:http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e06ea0dd9207
7121314: Behavior mismatch between AbstractCollection.toArray(T[] ) and its spec
Reviewed-by: dholmes, mduigou
Contributed-by: Sean Zhou<zhouyx at linux.vnet.ibm.com>, Ulf Zibis<ulf.zibis at gmx.de>, David Holmes<david.holmes at oracle.com>
! src/share/classes/java/util/AbstractCollection.java
+ test/java/util/AbstractCollection/ToArrayTest.java
Please verified it.
Thank you all for contributing and reviewing this change. Thank you Mike
and Joe for helping my committing this patch.
On 04/01/2012 05:24 PM, Sean Chou wrote:
> Hi David,
>
> I made the new webrev with your modified testcase:
> http://cr.openjdk.java.net/~zhouyx/7121314/webrev.04/ .
>
>
> The link to the bug:
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7121314
> The link to the start of the thread:
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-March/009512.html
>
>
> On Sun, Apr 1, 2012 at 11:25 AM, David Holmes<david.holmes at oracle.com>wrote:
>
>> Simplified testcase below.
>>
>> Let's finalise this please.
>>
>> Thanks,
>> David
>>
>>
>> On 1/04/2012 1:08 PM, Sean Chou wrote:
>>
>>> Hi Ulf,
>>>
>>> This is a regression testcase, there is no performance issue or
>>> future refactoring.
>>> Please wait for David's comments.
>>>
>>> On Sun, Apr 1, 2012 at 7:22 AM, Ulf Zibis<Ulf.Zibis at gmx.de
>>> <mailto:Ulf.Zibis at gmx.de>> wrote:
>>>
>>> Hi Sean,
>>>
>>> thanks for your effort.
>>>
>>> Am 31.03.2012 11:43, schrieb Sean Chou:
>>>
>>> Hi David and Ulf,
>>>
>>> The new webrev is at:
>>> http://cr.openjdk.java.net/~__**zhouyx/7121314/webrev.03/<http://cr.openjdk.java.net/~__zhouyx/7121314/webrev.03/>
>>> <http://cr.openjdk.java.net/~**zhouyx/7121314/webrev.03/<http://cr.openjdk.java.net/~zhouyx/7121314/webrev.03/>
>>> <http://cr.openjdk.java.net/%_**_7Ezhouyx/7121314/webrev.03/
>>>
>>> <http://cr.openjdk.java.net/%**7Ezhouyx/7121314/webrev.03/<http://cr.openjdk.java.net/%7Ezhouyx/7121314/webrev.03/>>>
>>> .
>>>
>>>
>>> About the fix, I remained the previous one.
>>> About the testcase, I merged the 3 files into one.
>>> During merging, there are 2 modifications:
>>> 1. I added static modifier to the 3 classes, which are enclosed
>>> by class ToArrayTest;
>>>
>>> You do not need the indirection via main()...run()...test() if you
>>> have all in 1 file. This was only necessary to feature a general
>>> usability of InfraStructure. You can go back to David's 1 + 1 nested
>>> class approach replacing TConcurrentHashMapX by TestCollection and
>>> maybe rename realMain() to test().
>>> Additionally, loading 4 classes for 1 test would have some
>>> performance impact on the test run, which could be avoided.
>>>
>>>
>>> 2. I removed field TestCollection.fixedSize, which is never read
>>> after Ulf fixed the bug in testcase.
>>>
>>> This field would serve to "reset" the TestCollection to fixed
>>> default size without the need of new instantiation for later
>>> refactoring or testcase addition.
>>>
>>> As just discussed before, the doc for setSizeSequence() could be
>>> little more specific:
>>> 71 /*
>>> 72 * Sets the values that size() will return on each use.
>>> The first
>>> 73 * call to size will return sizes[0], then sizes[1]
>>> etc. This
>>> 74 * allows us to emulate a concurrent change to the
>>> contents of
>>> 75 * the collection without having to perform concurrent
>>> changes.
>>> 76 * If sizes[n+1] contains a larger value than on last
>>> n-th invocation,
>>> 77 * the collection will appear to have shrunk when
>>> iterated; if a
>>> 78 * smaller value then the collection will appear to
>>> have grown.
>>> 79 * When the last element of sizes is reached, the
>>> collection will
>>> 80 * appear size-fixed.
>>> 81 */
>>>
>>>
>>> The link to the bug:
>>> http://bugs.sun.com/__**bugdatabase/view_bug.do?bug___**id=7121314<http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=7121314>
>>>
>>> <http://bugs.sun.com/**bugdatabase/view_bug.do?bug_**id=7121314<http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7121314>
>>> The link to the start of the thread:
>>> http://mail.openjdk.java.net/_**_pipermail/core-libs-dev/2012-**
>>> __March/009512.html<http://mail.openjdk.java.net/__pipermail/core-libs-dev/2012-__March/009512.html>
>>>
>>> <http://mail.openjdk.java.net/**pipermail/core-libs-dev/2012-**
>>> March/009512.html<http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-March/009512.html>
>>> Good idea, to repeat these links.
>>>
>>> -Ulf
>>>
>>>
>>>
>>>
>>> --
>>> Best Regards,
>>> Sean Chou
>>>
>>>
>> /*
>> * Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved.
>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>> *
>> * This code is free software; you can redistribute it and/or modify it
>> * under the terms of the GNU General Public License version 2 only, as
>> * published by the Free Software Foundation.
>> *
>> * This code is distributed in the hope that it will be useful, but WITHOUT
>> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>> * version 2 for more details (a copy is included in the LICENSE file that
>> * accompanied this code).
>> *
>> * You should have received a copy of the GNU General Public License
>> version
>> * 2 along with this work; if not, write to the Free Software Foundation,
>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>> *
>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>> * or visit www.oracle.com if you need additional information or have any
>> * questions.
>> */
>>
>> /*
>> * @test
>> * @bug 7121314
>> * @summary AbstractCollection.toArray(T[]**) doesn't return the given
>> array
>> * in concurrent modification.
>> * @author Ulf Zibis, David Holmes
>> */
>>
>> import java.util.AbstractCollection;
>> import java.util.Arrays;
>> import java.util.Iterator;
>>
>> public class ToArrayTest {
>>
>> static class TestCollection<E> extends AbstractCollection<E> {
>>
>> private final E[] elements;
>>
>>
>> private int[] sizes;
>> private int nextSize;
>>
>> public TestCollection(E[] elements) {
>> this.elements = elements;
>> setSizeSequence(new int[]{elements.length});
>> }
>>
>> /*
>> * Sets the values that size() will return on each use. The next
>>
>> * call to size will return sizes[0], then sizes[1] etc. This
>> * allows us to emulate a concurrent change to the contents of
>> * the collection without having to perform concurrent changes.
>> * If sizes[n+1] contains a larger value, the collection will
>> appear to
>> * have shrunk when iterated; if a smaller value then the
>> * collection will appear to have grown when iterated
>>
>> */
>> void setSizeSequence(int... sizes) {
>> this.sizes = sizes;
>> nextSize = 0;
>> }
>>
>> /* can change collection's size after each invocation */
>>
>> @Override
>> public int size() {
>> return sizes[nextSize == sizes.length-1 ? nextSize :
>> nextSize++];
>> }
>>
>> @Override
>> public Iterator<E> iterator() {
>> return new Iterator<E>() {
>>
>> int pos = 0;
>> public boolean hasNext() {
>> return pos< sizes[nextSize];
>> }
>> public E next() {
>> return elements[pos++];
>> }
>> public void remove() {
>> throw new UnsupportedOperationException(**"Not
>> supported yet.");
>> }
>> };
>> }
>> }
>>
>> static final Object[] OBJECTS = { new Object(), new Object(), new
>> Object() };
>> static final TestCollection<?> CANDIDATE = new TestCollection<Object>(**
>> OBJECTS);
>> static final int CAP = OBJECTS.length; // capacity of the CANDIDATE
>> static final int LAST = CAP - 1; // last possible array index
>> Object[] a;
>> Object[] res;
>>
>> int last() { return a.length - 1; }
>>
>> protected void test() throws Throwable {
>> // Check array type conversion
>> res = new TestCollection<>(new Object[]{"1", "2"}).toArray(new
>> String[0]);
>> check(res instanceof String[]);
>> check(res.length == 2);
>> check(res[1] == "2");
>>
>> // Check incompatible type of target array
>> try {
>> res = CANDIDATE.toArray(new String[CAP]);
>> check(false);
>> } catch (Throwable t) {
>> check(t instanceof ArrayStoreException);
>> }
>>
>> // Check more elements than a.length
>> a = new Object[CAP-1]; // appears too small
>> res = CANDIDATE.toArray(a);
>> check(res != a);
>> check(res[LAST] != null);
>>
>> // Check equal elements as a.length
>> a = new Object[CAP]; // appears to match
>> res = CANDIDATE.toArray(a);
>> check(res == a);
>> check(res[last()] != null);
>>
>> // Check equal elements as a.length
>> a = new Object[CAP+1]; // appears too big
>> res = CANDIDATE.toArray(a);
>> check(res == a);
>> check(res[last()] == null);
>>
>> // Check less elements than expected, but more than a.length
>> a = new Object[CAP-2]; // appears too small
>> CANDIDATE.setSizeSequence(CAP, CAP-1);
>> res = CANDIDATE.toArray(a);
>> check(res != a);
>> check(res.length == CAP-1);
>> check(res[LAST-1] != null);
>>
>> // Check less elements than expected, but equal as a.length
>> a = Arrays.copyOf(OBJECTS, CAP); // appears to match
>> CANDIDATE.setSizeSequence(CAP, CAP-1);
>> res = CANDIDATE.toArray(a);
>> check(res == a);
>> check(res[last()] == null);
>>
>> // Check more elements than expected and more than a.length
>> a = new Object[CAP-1]; // appears to match
>> CANDIDATE.setSizeSequence(CAP-**1, CAP);
>> res = CANDIDATE.toArray(a);
>> check(res != a);
>> check(res[LAST] != null);
>>
>> // Check more elements than expected, but equal as a.length
>> a = new Object[CAP-1]; // appears to match
>> CANDIDATE.setSizeSequence(CAP-**2, CAP-1);
>> res = CANDIDATE.toArray(a);
>> check(res == a);
>> check(res[last()] != null);
>>
>> // Check more elements than expected, but less than a.length
>> a = Arrays.copyOf(OBJECTS, CAP); // appears to match
>> CANDIDATE.setSizeSequence(CAP-**2, CAP-1);
>> res = CANDIDATE.toArray(a);
>> check(res == a);
>> check(res[last()] == null);
>>
>> test_7121314();
>> }
>>
>> /*
>> * Major target of this testcase, bug 7121314.
>> */
>> protected void test_7121314() throws Throwable {
>> // Check equal elements as a.length, but less than expected
>> a = new Object[CAP-1]; // appears too small
>> CANDIDATE.setSizeSequence(CAP, CAP-1);
>> res = CANDIDATE.toArray(a);
>> check(res == a);
>> check(res[last()] != null);
>>
>> // Check less elements than a.length and less than expected
>> a = Arrays.copyOf(OBJECTS, CAP-1); // appears too small
>> CANDIDATE.setSizeSequence(CAP, CAP-2);
>> res = CANDIDATE.toArray(a);
>> check(res == a);
>> check(res[last()] == null);
>>
>> }
>>
>>
>> public static void main(String[] args) throws Throwable {
>> ToArrayTest testcase = new ToArrayTest();
>> try { testcase.test(); } catch (Throwable t) { unexpected(t); }
>>
>> System.out.printf("%nPassed = %d, failed = %d%n%n", passed, failed);
>> if (failed> 0) throw new Exception("Some tests failed");
>> }
>>
>> private static volatile int passed = 0, failed = 0;
>> private static void pass() { passed++; }
>> private static void fail() { failed++; Thread.dumpStack(); }
>> private static void fail(String msg) { System.out.println(msg); fail(); }
>> private static void unexpected(Throwable t) { failed++;
>> t.printStackTrace(); }
>>
>> static void check(boolean cond) { if (cond) pass(); else fail(); }
>> static void equals(Object x, Object y) {
>>
>> if (x == null ? y == null : x.equals(y)) pass();
>> else {System.out.println(x + " not equal to " + y); fail(); }}
>>
>> }
>>
>>
>>
>
--
Yours Charles
More information about the core-libs-dev
mailing list