RFR [8022584] Memory leak in some NetworkInterface methods

[Ivan Gerasimov]

Thank you Michael!

On 09.08.2013 2:19, Michael McMahon wrote:
> Ivan,
>
> Right, it's not worth trying to do the equivalent, whatever it is, for 
> Windows.
> The test is fine with me.
>
> Thanks
> Michael
>
> On 08/08/13 23:15, Ivan Gerasimov wrote:
>> Michael,
>>
>> The test uses /proc/self/stat file to detect changes in virtual 
>> memory usage.
>> This approach is specific for Linux.
>> That's why I included the check of OS in the test.
>>
>> Sincerely yours,
>> Ivan
>>
>> On 09.08.2013 1:38, Michael McMahon wrote:
>>> Yes, definitely "othervm" would be required for the test. Also, why 
>>> skip other OS'es?
>>> The fix is only for Linux, but it might catch future leaks on Windows.
>>>
>>> The trouble with tests like this, is they sometimes don't age well.
>>> Future changes in OS kernel behavior could cause problems but I 
>>> guess 32MB is a fairly large difference.
>>> So, it should be okay
>>>
>>> Michael
>>>
>>> On 08/08/13 22:15, David Holmes wrote:
>>>> Main fix looks good to me.
>>>>
>>>> Regression test may need some tweaking eg I think othervm will be 
>>>> needed.
>>>>
>>>> Also this:
>>>>
>>>>  System.out.println("WARNING: Cannot perform memory leak detection 
>>>> on this OS");
>>>>
>>>> should probably just say something like "Test skipped on this OS" - 
>>>> there are other tests that do this so just check if there is common 
>>>> terminology. (In the future we may have keyword tags to flag this 
>>>> as linux only etc.)
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 9/08/2013 4:41 AM, Ivan Gerasimov wrote:
>>>>> Chris, if it's not too late, I'd like to include a regtest in the 
>>>>> fix.
>>>>>
>>>>> Here's webrev that includes the test:
>>>>> http://cr.openjdk.java.net/~igerasim/8022584/3/webrev/
>>>>>
>>>>> The test gets past with the fixed jdk8 and fails with jdk8-b101 
>>>>> and jdk7
>>>>> as expected.
>>>>>
>>>>> Sincerely yours,
>>>>> Ivan
>>>>>
>>>>> On 08.08.2013 17:50, Chris Hegarty wrote:
>>>>>> Looks good to me. Thanks Ivan.
>>>>>>
>>>>>> -Chris.
>>>>>>
>>>>>> On 08/08/2013 02:33 PM, Ivan Gerasimov wrote:
>>>>>>> Hello Chris!
>>>>>>>
>>>>>>> Here's the update:
>>>>>>> http://cr.openjdk.java.net/~igerasim/8022584/2/webrev/
>>>>>>>
>>>>>>> Thanks for offering the sponsorship!
>>>>>>> Here's the hg-export
>>>>>>> http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Sincerely yours,
>>>>>>> Ivan
>>>>>>>
>>>>>>> On 08.08.2013 12:43, Chris Hegarty wrote:
>>>>>>>> Thanks for taking this Ivan.
>>>>>>>>
>>>>>>>> Can you please make the changes suggested by both David and Alan (
>>>>>>>> simply return NULL/-1/JNI_FALSE, as appropriate, if 
>>>>>>>> GetStringUTFChars
>>>>>>>> fails ( returns NULL ), then I will sponsor this change into 
>>>>>>>> jdk8 for
>>>>>>>> you.
>>>>>>>>
>>>>>>>> Please post an update webrev to cr.openjdk.java.net.
>>>>>>>>
>>>>>>>> -Chris.
>>>>>>>>
>>>>>>>> On 08/08/2013 06:01 AM, David Holmes wrote:
>>>>>>>>> Ivan,
>>>>>>>>>
>>>>>>>>> On 8/08/2013 2:05 PM, Ivan Gerasimov wrote:
>>>>>>>>>> David, Alan,
>>>>>>>>>>
>>>>>>>>>> I added checking for NULL  results and throwing OOMException if
>>>>>>>>>> necessary.
>>>>>>>>>
>>>>>>>>> You don't need to throw it yourself:
>>>>>>>>>
>>>>>>>>>    JNU_ThrowOutOfMemoryError(env, NULL);
>>>>>>>>>
>>>>>>>>> Assuming a correct VM implementation if NULL is returned then 
>>>>>>>>> an OOME
>>>>>>>>> should already be pending and will be thrown as soon as your 
>>>>>>>>> native
>>>>>>>>> code
>>>>>>>>> returns to Java.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>> I've also added some spaces along the code to improve 
>>>>>>>>>> indentation.
>>>>>>>>>>
>>>>>>>>>> Would you please review the updated webrev?
>>>>>>>>>> http://washi.ru.oracle.com/~igerasim/webrevs/8022584/1/webrev/
>>>>>>>>>>
>>>>>>>>>> Sincerely yours,
>>>>>>>>>> Ivan
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 08.08.2013 5:35, David Holmes wrote:
>>>>>>>>>>> On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:
>>>>>>>>>>>> Thanks Alan!
>>>>>>>>>>>>
>>>>>>>>>>>> I've checked that and it turns out that GetStringUTFChars 
>>>>>>>>>>>> cannot
>>>>>>>>>>>> return
>>>>>>>>>>>> NULL.
>>>>>>>>>>>> For allocation of the result string it calls AllocateHeap() 
>>>>>>>>>>>> with
>>>>>>>>>>>> the
>>>>>>>>>>>> default EXIT_OOM fail strategy.
>>>>>>>>>>>> Thus, in case of being unable to allocate memory it simply 
>>>>>>>>>>>> stops
>>>>>>>>>>>> VM.
>>>>>>>>>>>
>>>>>>>>>>> Ouch! That is a hotspot bug. GetStringUTFChars is supposed 
>>>>>>>>>>> to throw
>>>>>>>>>>> OutOfMemoryError if it has memory issues, not abort the VM!
>>>>>>>>>>>
>>>>>>>>>>> I recommend that you check for NULL and/or a pending exception.
>>>>>>>>>>>
>>>>>>>>>>> David
>>>>>>>>>>>
>>>>>>>>>>>> Sincerely yours,
>>>>>>>>>>>> Ivan
>>>>>>>>>>>>
>>>>>>>>>>>> On 08.08.2013 5:05, Alan Bateman wrote:
>>>>>>>>>>>>> (cc'ing net-dev).
>>>>>>>>>>>>>
>>>>>>>>>>>>> The change looks okay to me. One suggestion (while you are
>>>>>>>>>>>>> there) is
>>>>>>>>>>>>> to check the return from GetStringUTFChars so that the name
>>>>>>>>>>>>> returns
>>>>>>>>>>>>> when it fails with NULL.
>>>>>>>>>>>>>
>>>>>>>>>>>>> -Alan.
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 07/08/2013 17:46, Ivan Gerasimov wrote:
>>>>>>>>>>>>>> Hello everybody!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Some methods of NetworkInterface class were reported to leak
>>>>>>>>>>>>>> memory.
>>>>>>>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet
>>>>>>>>>>>>>> available.)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Examples are isUp() and isLoopback().
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Affected versions of jdk are 6, 7 and 8
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Would you please review a simple fix that removes the 
>>>>>>>>>>>>>> unnecessary
>>>>>>>>>>>>>> allocation?
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Sincerely yours,
>>>>>>>>>>>>>> Ivan
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>>>
>>>
>>
>
>
>