RFR: 8072777: java/lang/ref/ReferenceEnqueuePending.java: some references aren't queued

[Peter Levart]

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