[PATCH] 6822627: NPE at ReferenceTypeImpl.constantPool

[serguei.spitsyn at oracle.com]

Hi Egor,

Thank you for providing the test!

A couple of nits to the test:

   56             if (!Arrays.equals(cpbytes, cpbytes2)) {
   57                 failure("Consequent constantPool results vary, first was : " + cpbytes + ", now: " + cpbytes2);
   58             };

    Last semicolon is not need.

   74         if (!testFailed) {
   75             println("ConstantPoolInfoGC: passed");
   76         } else {
   77             throw new Exception("ConstantPoolInfoGC: failed");
   78         }

   I'd suggest to rearrange the statement above to something like this:

   74         if (testFailed) {
   75             throw new Exception("ConstantPoolInfoGC: failed");
   76         }
   77         println("ConstantPoolInfoGC: passed");

    But I leave it up to you.    No need to see another webrev.    I can 
sponsor your fix, if needed. Thanks, Serguei On 7/19/16 00:07, Egor 
Ushakov wrote:
>
> Hi Serguei,
>
> here's a new webrev with a test included: 
> http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.01/
>
> Egor
>
> On 15.07.2016 12:22, serguei.spitsyn at oracle.com wrote:
>> Hi Egor, The fix looks good modulo the synchronization issue. Do you 
>> have a jtreg unit test reproducing this failure mode? We have a rule 
>> to provide a test coverage for the fixes. Thanks, Serguei On 7/15/16 
>> 00:03, Egor Ushakov wrote:
>>>
>>> Thanks for your comments!
>>>
>>> I totally agree that the code there requires major refactoring. In 
>>> the fix I tried not to make it worse and fix the NPE.
>>>
>>> On 14.07.2016 20:06, Martin Buchholz wrote:
>>>> The lack of volatile or synchronization, plus the use of multiple 
>>>> mutable fields, suggests that a deeper fix is necessary to be truly 
>>>> correct.  For comparison, much of the similar code implementing 
>>>> reflection has been changed to use volatile.  But that's a big job, 
>>>> for the serviceability team.
>>>> On Thu, Jul 14, 2016 at 2:33 AM, Egor Ushakov 
>>>> <egor.ushakov at jetbrains.com <mailto:egor.ushakov at jetbrains.com>> 
>>>> wrote:
>>>>
>>>>     Hi, I'm a developer from IDEA debugger (we have company OCA).
>>>>     We've increased usage of ReferenceTypeImpl.constantPool and now
>>>>     facing JDK-6822627 more often. Please review and sponsor the
>>>>     fix. The problem was that constantPoolBytesRef SoftReference
>>>>     value coud be GCed and there was no check for that. I've added
>>>>     it to the check inside getConstantPoolInfo to request the bytes
>>>>     again in this case. BUGURL:
>>>>     https://bugs.openjdk.java.net/browse/JDK-6822627 WEBREV:
>>>>     http://cr.openjdk.java.net/~avu/JDK-6822627/webrev.00/
>>>>     <http://cr.openjdk.java.net/%7Eavu/JDK-6822627/webrev.00/>
>>>>     Thanks!-- Egor Ushakov Software Developer JetBrains
>>>>     http://www.jetbrains.com The Drive to Develop 
>>>>
>>> -- 
>>> Egor Ushakov
>>> Software Developer
>>> JetBrains
>>> http://www.jetbrains.com
>>> The Drive to Develop
> -- 
> Egor Ushakov
> Software Developer
> JetBrains
> http://www.jetbrains.com
> The Drive to Develop
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160720/e51648be/attachment.html>