Tweak/Fix ObjectTest.
This CL combines misc tweaks/refactorings and fixes.
It's not clear whether ObjectTest#test_notify() is flaky
because 200msec isn't enough time for the other thread to
wake up, or if there is some other bugs. For example, the
test previously didn't deal correctly with spurious wake-ups.
Fixes:
- The tests were not dealing with spurious wake-up of the wait()ing
background threads. This CL introduces the field
outstandingNotifications to limit the number of tests that
may wake up to how many were notified.
- Increase the amount of time that test_notify() waits for the
notify'ed thread to take action from 200msec to 400msec.
- Replaced background exception signaling via a global field
backgroundException that is checked during tearDown, rather
than by setting status. The field is also guarded by lock to
avoid introducing a use of volatile/AtomicReference to the
test.
Tweaks:
- Replace a few instances of assertTrue(..., ... == ...) with
assertEquals() or assertSame().
- When waiting for something to happen, sleep for a short while
first and only sleep more if the thing hasn't happened yet.
(Normally this kind of code would be rewritten using a
CountDownLatch, but I didn't want to change the test by
introducing other classes that affect threading semantics).
- make TestThread extend Thread rather than Runnable.
- Move the field status into the remaining to Thread classes
that need it.
- Rename obj1 to lock for use by the tests that use a synchronized
block, and move obj1, obj2 into the tests that need those.
- Slight reformatting for conciseness.
- Drop some redundant lines.
Test: atest --iterations 20 \
CtsLibcoreTestCases:org.apache.harmony.tests.java.lang.ObjectTest
Test: Ran the following on a Pixel 2; each of the 500 test runs took
a bit over 2 seconds to complete because we sleep for at least
100msec waiting for each of the 20 background threads to wake
up and exit.
atest --iterations 100 \
CtsLibcoreTestCases:org.apache.harmony.tests.java.lang.ObjectTest#test_notify
Bug: 144254071
Change-Id: I4cea62b2abde9c06f44e0c8754003ead84d00c00
diff --git a/harmony-tests/src/test/java/org/apache/harmony/tests/java/lang/ObjectTest.java b/harmony-tests/src/test/java/org/apache/harmony/tests/java/lang/ObjectTest.java
index 9dca8e9..f8cff0d 100644
--- a/harmony-tests/src/test/java/org/apache/harmony/tests/java/lang/ObjectTest.java
+++ b/harmony-tests/src/test/java/org/apache/harmony/tests/java/lang/ObjectTest.java
@@ -16,34 +16,52 @@
*/
package org.apache.harmony.tests.java.lang;
+import java.util.ArrayList;
+import java.util.List;
+
public class ObjectTest extends junit.framework.TestCase {
- /**
- * Test objects.
- */
- Object obj1 = new Object();
+ final Object lock = new Object();
- Object obj2 = new Object();
-
- /**
- * Generic state indicator.
- */
- int status = 0;
-
+ // Helpers for test_notify() and test_notifyAll(). Access is guarded by {@code lock}'s monitor.
int ready = 0;
+ int finished = 0;
+ int outstandingNotifications = 0;
+ int spuriousNotifications = 0;
+ Throwable backgroundException;
+
+ @Override
+ protected void setUp() throws Exception {
+ super.setUp();
+ synchronized (lock) {
+ backgroundException = null;
+ }
+ }
+
+ @Override
+ protected void tearDown() throws Exception {
+ synchronized (lock) {
+ if (backgroundException != null) {
+ fail("Encountered " + backgroundException);
+ }
+ }
+ super.tearDown();
+ }
/**
* java.lang.Object#Object()
*/
- public void test_Constructor() {
+ public void test_constructor() {
// Test for method java.lang.Object()
- assertNotNull("Constructor failed !!!", new Object());
+ assertNotNull("Constructor failed", new Object());
}
/**
* java.lang.Object#equals(java.lang.Object)
*/
public void test_equalsLjava_lang_Object() {
+ Object obj1 = new Object();
+ Object obj2 = new Object();
// Test for method boolean java.lang.Object.equals(java.lang.Object)
assertTrue("Same object should be equal", obj1.equals(obj1));
assertTrue("Different objects should not be equal", !obj1.equals(obj2));
@@ -54,20 +72,15 @@
*/
public void test_getClass() throws Exception {
// Test for method java.lang.Class java.lang.Object.getClass()
- String classNames[] = { "java.lang.Object", "java.lang.Throwable",
+ String[] classNames = { "java.lang.Object", "java.lang.Throwable",
"java.lang.StringBuffer" };
- Class<?> classToTest = null;
- Object instanceToTest = null;
-
- status = 0;
- for (int i = 0; i < classNames.length; ++i) {
- classToTest = Class.forName(classNames[i]);
- instanceToTest = classToTest.newInstance();
- assertTrue("Instance didn't match creator class.",
- instanceToTest.getClass() == classToTest);
- assertTrue("Instance didn't match class with matching name.",
- instanceToTest.getClass() == Class
- .forName(classNames[i]));
+ for (String className : classNames) {
+ final Class<?> classToTest = Class.forName(className);
+ final Object instanceToTest = classToTest.newInstance();
+ assertSame("Instance didn't match creator class.", instanceToTest.getClass(),
+ classToTest);
+ assertSame("Instance didn't match class with matching name.", instanceToTest.getClass(),
+ Class.forName(className));
}
}
@@ -76,10 +89,19 @@
*/
public void test_hashCode() {
// Test for method int java.lang.Object.hashCode()
- assertTrue("Same object should have same hash.",
- obj1.hashCode() == obj1.hashCode());
- assertTrue("Same object should have same hash.",
- obj2.hashCode() == obj2.hashCode());
+ Object obj1 = new Object();
+ Object obj2;
+ int origHashCodeForObj1 = obj1.hashCode();
+ // Force obj1 lock inflation.
+ synchronized (obj1) {
+ obj2 = new Object();
+ }
+ assertEquals("Same object should have same hash.", obj1.hashCode(), obj1.hashCode());
+ assertEquals("Lock inflation shouldn't change hash code.",
+ obj1.hashCode(), origHashCodeForObj1);
+ assertEquals("Same object should have same hash.", obj2.hashCode(), obj2.hashCode());
+ Runtime.getRuntime().gc();
+ assertEquals("Gc shouldn't change hash code.", obj1.hashCode(), origHashCodeForObj1);
}
/**
@@ -87,67 +109,72 @@
*/
public void test_notify() {
// Test for method void java.lang.Object.notify()
+ class TestThread extends Thread {
+ public TestThread(String name) {
+ super(name);
+ }
- // Inner class to run test thread.
- class TestThread implements Runnable {
+ @Override
public void run() {
- synchronized (obj1) {
+ synchronized (lock) {
try {
- ready += 1;
- obj1.wait();// Wait for ever.
- status += 1;
+ ready++;
+ lock.wait(); // Wait to be notified.
+ while (outstandingNotifications <= 0) {
+ spuriousNotifications++;
+ lock.wait();
+ }
+ outstandingNotifications--;
} catch (InterruptedException ex) {
- status = -1000;
+ backgroundException = ex;
}
}
}
}
- ;
-
- // Start of test code.
// Warning:
// This code relies on each thread getting serviced within
- // 200 mSec of when it is notified. Although this
+ // 400 msec of when it is notified. Although this
// seems reasonable, it could lead to false-failures.
ready = 0;
- status = 0;
+ outstandingNotifications = 0;
+ spuriousNotifications = 0;
final int readyWaitSecs = 3;
final int threadCount = 20;
+ List<TestThread> threads = new ArrayList<>();
for (int i = 0; i < threadCount; ++i) {
- new Thread(new TestThread()).start();
+ TestThread thread = new TestThread("TestThread " + i);
+ threads.add(thread);
+ thread.start();
}
- synchronized (obj1) {
+ synchronized (lock) {
try {
-
// Wait up to readyWaitSeconds for all threads to be waiting on
// monitor
- for (int i = 0; i < readyWaitSecs; i++) {
- obj1.wait(1000, 0);
+ for (int i = 0; i < 10 * readyWaitSecs; i++) {
+ lock.wait(100, 0);
if (ready == threadCount) {
break;
}
}
-
- // Check pre-conditions of testing notifyAll
- assertTrue("Not all launched threads are waiting. (ready = "
- + ready + ")", ready == threadCount);
- assertTrue("Thread woke too early. (status = " + status + ")",
- status == 0);
-
+ assertEquals("Not all launched threads are waiting. (ready=" + ready + ")",
+ ready, threadCount);
for (int i = 1; i <= threadCount; ++i) {
- obj1.notify();
- obj1.wait(200, 0);
- assertTrue("Out of sync. (expected " + i + " but got "
- + status + ")", status == i);
+ outstandingNotifications++;
+ lock.notify();
+ for (int j = 0; j < 10 && outstandingNotifications > 0; ++j) {
+ lock.wait(100); // Sleep for 100 msecs, releasing lock.
+ }
+ assertEquals("Notification #" + i + " took too long to wake a thread.",
+ 0, outstandingNotifications);
+ // Spurious notifications are allowed, but should be very rare.
+ assertTrue("Too many spurious notifications: " + spuriousNotifications,
+ spuriousNotifications <= 1);
}
-
} catch (InterruptedException ex) {
- fail(
- "Unexpectedly got an InterruptedException. (status = "
- + status + ")");
+ fail("Unexpectedly got an InterruptedException.");
}
}
}
@@ -161,13 +188,13 @@
// Inner class to run test thread.
class TestThread implements Runnable {
public void run() {
- synchronized (obj1) {
+ synchronized (lock) {
try {
ready += 1;
- obj1.wait();// Wait for ever.
- status += 1;
+ lock.wait();// Wait forever.
+ finished += 1;
} catch (InterruptedException ex) {
- status = -1000;
+ backgroundException = ex;
}
}
}
@@ -181,45 +208,43 @@
// 5 seconds of when they are notified. Although this
// seems reasonable, it could lead to false-failures.
- status = 0;
ready = 0;
+ finished = 0;
final int readyWaitSecs = 3;
+ final int finishedWaitSecs = 5;
final int threadCount = 20;
for (int i = 0; i < threadCount; ++i) {
new Thread(new TestThread()).start();
}
- synchronized (obj1) {
+ synchronized (lock) {
try {
-
// Wait up to readyWaitSeconds for all threads to be waiting on
// monitor
for (int i = 0; i < readyWaitSecs; i++) {
- obj1.wait(1000, 0);
+ lock.wait(1000, 0);
if (ready == threadCount) {
break;
}
}
// Check pre-conditions of testing notifyAll
- assertTrue("Not all launched threads are waiting. (ready = "
- + ready + ")", ready == threadCount);
- assertTrue("At least one thread woke too early. (status = "
- + status + ")", status == 0);
+ assertEquals("Not all launched threads are waiting.", threadCount, ready);
+ // This assumes no spurious wakeups. If we ever see any, we might check for at
+ // most one finished thread instead.
+ assertEquals("At least one thread woke too early.", 0, finished);
- obj1.notifyAll();
+ lock.notifyAll();
- obj1.wait(5000, 0);
+ for (int i = 0; finished < threadCount && i < finishedWaitSecs; ++i) {
+ lock.wait(1000, 0);
+ }
- assertTrue(
- "At least one thread did not get notified. (status = "
- + status + ")", status == threadCount);
+ assertEquals("At least one thread did not get notified.", threadCount, finished);
} catch (InterruptedException ex) {
- fail(
- "Unexpectedly got an InterruptedException. (status = "
- + status + ")");
+ fail("Unexpectedly got an InterruptedException. (finished = " + finished + ")");
}
}
@@ -230,7 +255,7 @@
*/
public void test_toString() {
// Test for method java.lang.String java.lang.Object.toString()
- assertNotNull("Object toString returned null.", obj1.toString());
+ assertNotNull("Object toString returned null.", lock.toString());
}
/**
@@ -240,19 +265,23 @@
// Test for method void java.lang.Object.wait()
// Inner class to run test thread.
- class TestThread implements Runnable {
+ class TestThread extends Thread {
+ int status;
+
public void run() {
- synchronized (obj1) {
+ synchronized (lock) {
try {
- obj1.wait();// Wait for ever.
+ do {
+ lock.wait(); // Wait to be notified.
+ } while (outstandingNotifications <= 0);
+ outstandingNotifications--;
status = 1;
} catch (InterruptedException ex) {
- status = -1;
+ backgroundException = ex;
}
}
}
}
- ;
// Start of test code.
@@ -261,21 +290,23 @@
// 1 second of when they are notified. Although this
// seems reasonable, it could lead to false-failures.
- status = 0;
- new Thread(new TestThread()).start();
- synchronized (obj1) {
+ TestThread thread = new TestThread();
+ synchronized (lock) {
+ thread.status = 0;
+ }
+ thread.start();
+ synchronized (lock) {
try {
- obj1.wait(1000, 0);
- assertTrue("Thread woke too early. (status = " + status + ")",
- status == 0);
- obj1.notifyAll();
- obj1.wait(1000, 0);
- assertTrue("Thread did not get notified. (status = " + status
- + ")", status == 1);
+ lock.wait(1000, 0);
+ assertEquals("Thread woke too early. (status=" + thread.status + ")",
+ 0, thread.status);
+ outstandingNotifications = 1;
+ lock.notifyAll();
+ lock.wait(1000, 0);
+ assertEquals("Thread did not get notified. (status=" + thread.status + ")",
+ 1, thread.status);
} catch (InterruptedException ex) {
- fail(
- "Unexpectedly got an InterruptedException. (status = "
- + status + ")");
+ fail("Unexpectedly got an InterruptedException. (status=" + thread.status + ")");
}
}
}
@@ -289,15 +320,15 @@
// Start of test code.
final int loopCount = 20;
- final int allowableError = 100; // millesconds
+ final int allowableError = 100; // milliseconds
final int delay = 200; // milliseconds
- synchronized (obj1) {
+ synchronized (lock) {
try {
int count = 0;
long[][] toLong = new long[3][3];
for (int i = 0; i < loopCount; ++i) {
long before = System.currentTimeMillis();
- obj1.wait(delay, 0);
+ lock.wait(delay, 0);
long after = System.currentTimeMillis();
long error = (after - before - delay);
if (error < 0)
@@ -311,10 +342,9 @@
count++;
}
if (error > (1000 + delay) || count == toLong.length) {
- StringBuffer sb = new StringBuffer();
+ StringBuilder sb = new StringBuilder();
for (int j = 0; j < count; j++) {
- sb
- .append("wakeup time too inaccurate, iteration ");
+ sb.append("wakeup time too inaccurate, iteration ");
sb.append(toLong[j][0]);
sb.append(", before: ");
sb.append(toLong[j][1]);
@@ -329,9 +359,7 @@
}
}
} catch (InterruptedException ex) {
- fail(
- "Unexpectedly got an InterruptedException. (status = "
- + status + ")");
+ fail("Unexpectedly got an InterruptedException.");
}
}
}
@@ -343,21 +371,26 @@
// Test for method void java.lang.Object.wait(long, int)
// Inner class to run test thread.
- class TestThread implements Runnable {
+ class TestThread extends Thread {
+ int status;
+
+ @Override
public void run() {
- synchronized (obj1) {
+ synchronized (lock) {
try {
- obj1.wait(0, 1); // Don't wait very long.
+ lock.wait(0, 1); // Don't wait very long.
status = 1;
- obj1.wait(0, 0); // Wait for ever.
+ do {
+ lock.wait(0, 0); // Wait to be notified.
+ } while (outstandingNotifications <= 0);
+ outstandingNotifications--;
status = 2;
} catch (InterruptedException ex) {
- status = -1;
+ backgroundException = ex;
}
}
}
}
- ;
// Start of test code.
@@ -366,23 +399,25 @@
// 1 second of when they are notified. Although this
// seems reasonable, it could lead to false-failures.
- status = 0;
- new Thread(new TestThread()).start();
- synchronized (obj1) {
+ TestThread thread = new TestThread();
+ synchronized (lock) {
+ thread.status = 0;
+ }
+ thread.start();
+ synchronized (lock) {
try {
- obj1.wait(1000, 0);
- assertTrue("Thread did not wake after 1 ms. (status = "
- + status + ")", status == 1);
- obj1.notifyAll();
- obj1.wait(1000, 0);
- assertTrue("Thread did not get notified. (status = " + status
- + ")", status == 2);
+ lock.wait(1000, 0);
+ assertEquals("Thread did not wake after 1sec. (status=" + thread.status + ")",
+ 1, thread.status);
+ outstandingNotifications++;
+ lock.notifyAll();
+ lock.wait(1000, 0);
+ assertEquals("Thread did not get notified. (status=" +
+ thread.status + ")", 2, thread.status);
} catch (InterruptedException ex) {
- fail(
- "Unexpectedly got an InterruptedException. (status = "
- + status + ")");
+ fail("Unexpectedly got an InterruptedException. (status = " +
+ thread.status + ")");
}
}
-
}
}