Hi Kim,
This change will make it practically impossible to miss the enqueued
references. But this could be an opportunity to also clean-up the code a
bit. The checkResult method has an 'obj' parameter whose purpose is not
immediately obvious. It's only used in the following line:
136 // Use the last Reference object of those created above,
so as to keep it "alive".
137 System.out.println("I must write " + obj + " to prevent
compiler optimizations.");
...the comment is misleading. If we look at how checkResult is called,
we can see that it is not the last Reference object that is passed to
this parameter, but its referent (an Integer object). This is the
referent of the Reference that is not supposed to be enqueued as
checkResult is called with parameters (refQueue, obj, iterations-1),
which means that it is expected to retrieve one less Reference object
than the number of Reference objects created.
I don't know why such complications are necessary. Is the test supposed
to also verify the requirement that a Reference whose referent is kept
strongly reachable will not be enqueued? If that is true, then there are
two things that should be fixed to ensure this requirement is actually
tested:
- the last Reference object that contains a referent that is kept
strongly reachable should also be kept strongly reachable while the
checkResult method is dequeuing References or else it can be GCed before
actually being discovered.
- the checkReference method should also wait for some time for the last
Reference to be enqueued. It is not necessary to wait 30 seconds. 2
seconds should be enough to not miss this last Reference if it was
enqueued as a result of some bug.
As to the implementation, it is not necessary to pass the last Reference
or it's referent to the checkReference method to keep them strongly
reachable. checkReference method does not have to deal with that. The
last Reference and it's referent could be kept strongly reachable in the
following way:
102 // Verify that all WeakReference objects (but last one)
ended up queued.
103 checkResult(refQueue, iterations-1);
// Keep the last Reference object and its referent
alive until after the ckeck
Reference.reachabilityFence(obj);
Reference.reachabilityFence(weaky);
104 System.out.println("Test passed.");
Regards, Peter
On 01/31/2016 05:37 AM, Kim Barrett wrote:
Please review this fix of a bug in the ReferenceEnqueuePending test.
The original code assumes the pending list processor will stay ahead
of the test thread, so that the test's polling of the reference queue
won't find it empty while there are still references on the internal
pending list. That might not always happen though, possibly in part
because the test raises the priority of its own thread to better
compete with the pending list processor.
The fix is to use queue.remove with a large timeout, rather than
queue.poll, to pull references from the queue until we've obtained all
we expect to get. Then switch to polling to verify nothing unexpected
in the queue without making the test delay for an expected timeout. If
all of the references are (eventually) queued as expected then the
timeout will never come close to expiring.
I also looked at uses of poll() by other tests in the same directory,
and didn't find any that looked like they might have a similar problem
of failing because the pending list processor had not yet gotten
around to dealing with a reference.
CR:
https://bugs.openjdk.java.net/browse/JDK-8072777
Webrev:
http://cr.openjdk.java.net/~kbarrett/8072777/webrev.00/
Testing:
Local testing of before and after test code, with an occasional sleep
inserted into the pending list processor's loop to simulate a
scheduling delay. With that delay, the original test frequently fails,
while the modified test consistently passes.
