Thanks David!
On 09.08.2013 1:15, David Holmes wrote:
Main fix looks good to me.
Regression test may need some tweaking eg I think othervm will be needed.
Yes, it's a good point.
Since there may be a memory leak in the test, it'd better not interfere
with other tests in jtreg.
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.)
OK, I changed the phrase to "Test only runs on Linux".
Updated webrev is here:
http://cr.openjdk.java.net/~igerasim/8022584/4/webrev/
Updated export is at the same place:
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch
Sincerely yours,
Ivan
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
