<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body style="background-color: rgb(255, 255, 255); color: rgb(0, 0,
0);" bgcolor="#FFFFFF" text="#000000">
Hi Kim,<br>
<br>
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:<br>
<br>
136 // Use the last Reference object of those created
above, so as to keep it "alive".<br>
137 System.out.println("I must write " + obj + " to prevent
compiler optimizations.");<br>
<br>
...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.<br>
<br>
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:<br>
<br>
- 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.<br>
- 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.<br>
<br>
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:<br>
<br>
102 // Verify that all WeakReference objects (but last one)
ended up queued.<br>
103 checkResult(refQueue, iterations-1);<br>
<br>
// Keep the last Reference object and its referent
alive until after the ckeck<br>
Reference.reachabilityFence(obj);<br>
Reference.reachabilityFence(weaky);<br>
<br>
104 System.out.println("Test passed.");<br>
<br>
<br>
<br>
Regards, Peter<br>
<br>
<div class="moz-cite-prefix">On 01/31/2016 05:37 AM, Kim Barrett
wrote:<br>
</div>
<blockquote
cite="mid:9C4B1F92-29F6-4977-BB48-342ADF886B56@oracle.com"
type="cite"><!--[if !IE]><DIV style="border-left: 2px solid #009900; border-right: 2px solid #009900; padding: 0px 15px; margin: 2px 0px;"><![endif]-->
<pre wrap="">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:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8072777">https://bugs.openjdk.java.net/browse/JDK-8072777</a>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~kbarrett/8072777/webrev.00/">http://cr.openjdk.java.net/~kbarrett/8072777/webrev.00/</a>
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.
</pre>
<!--[if !IE]></DIV><![endif]--></blockquote>
<br>
</body>
</html>