Re: RFR [8022584] Memory leak in some NetworkInterface methods
David, Chris,
I reverted back NULL-checking.
Now the change consists of one line removal and a regression test.
Webrev: http://cr.openjdk.java.net/~igerasim/8022584/6/webrev/
Hg export:
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch
Sincerely yours,
Ivan
On 09.08.2013 16:18, David Holmes wrote:
Hi Chris,
On 9/08/2013 8:36 PM, Chris Hegarty wrote:
Firstly, I think the memory leak issue should be moved forward
separately to this cleanup effort. They are unrelated, and I'm starting
to get the feeling that this could take some time to reach conclusion.
It seems reasonable to separate the issues.
I agree. I'm sure when Alan suggested to check the return he didn't
expect it to unravel like this :) As we know hotspot will never
actually return NULL there is no urgency to add this in.
On 09/08/2013 10:27, Ivan Gerasimov wrote:
Chris,
I would use this
if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
JNU_ThrowOutOfMemoryError(env, "GetStringUTFChars failed");
return NULL/JNI_False/-1;
}
If I understand it correctly, JNU_ThrowOutOfMemoryError throws an
exception only if it hasn't been already thrown.
JNU_ThrowOutOfMemoryError is simply a convenience wrapper for
JNU_ThrowByName(env, "java/lang/OutOfMemoryError", msg);
...and JNU_ThrowByName [1] is defined as...
JNU_ThrowByName(JNIEnv *env, const char *name, const char *msg) {
class cls = (*env)->FindClass(env, name);
if (cls != 0) /* Otherwise an exception has already been thrown */
(*env)->ThrowNew(env, cls, msg);
}
}
Neither FindClass or ThrowNew is safe to call if there is a pending
exception [1].
Right - we have to check for a pending exception before trying to
throw one.
Now the issue comes down to; could there ever be a pending exception if
GetStringUTFChars returns NULL? The latest specs doesn't indicate that
there could be, but every copy of "The Java Native Interface
Programmer's Guide and Specification" I can find does. There also
appears to be an assumption of this if you look at the usages in the
JDK.
AFAIK there is only one version of the JNI spec book and it never got
updated. The official spec says no throw, but when people have the
book on their bookshelf that is what they tend to rely on. I looked at
some of the usages and they seem exception agnostic - many of them
don't even check for NULL :(
The implementation as it stands will not throw and will not return NULL.
I would really like to get a definitive answer on the JNI specification
for GetStringUTFChars before making any changes here.
The JNI spec (as opposed to the book) is definitive. If we don't like
what is there then it requires a spec change.
I can't find any reference to this particular issue being raised before.
Cheers,
David
-Chris.
[1]
http://hg.openjdk.java.net/jdk8/tl/jdk/file/54f0ccdd9ad7/src/share/native/common/jni_util.c
[2]
http://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp17626
Sincerely yours,
Ivan
On 09.08.2013 11:25, Chris Hegarty wrote:
On 09/08/2013 06:47, David Holmes wrote:
Sorry I messed this up. The JNI book says GetStringUTFChars will
return
NULL and post OOME but the JNI spec (latest version 6.0) does not
- it
only says it will return NULL on failure.
This is indeed strange. Most usages of this function in the jdk expect
the former. If this is not the case, then we may need to do an audit
of all usages.
So your previous version was the more correct. Given we just
failed to
allocate C-heap I think we are on thin ice anyway, but better to at
least attempt to do the right thing.
I'm not sure what the right thing to do here is? It seems a little
unwieldy!
if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
if ((*env)->ExceptionOccurred(env)) {
return NULL/JNI_False/-1;
} else {
throwException(env, "java/lang/InternalError", "GetStringUTFChars
failed");
return NULL/JNI_False/-1;
}
Given we have no idea why GetStringUTFChars may have failed, what
exception do we throw?
Also worth noting is that this bug fix has moved away from the
original problem (memory leak), and is now focused on code cleanup.
If we cannot get agreement on the cleanup, or it looks like more
clarification is needed around the cleanup effort, then I would like
to suggest that we proceed with the original fix for the memory leak
and separate out the cleanup effort.
-Chris.
FYI I filed 8022683 to fix GetStringUTFChars.
David
-
On 9/08/2013 3:21 PM, David Holmes wrote:
Thumbs up!
Thanks,
David
On 9/08/2013 8:19 AM, Ivan Gerasimov wrote:
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 per
Re: RFR [8022584] Memory leak in some NetworkInterface methods
Thank you Ivan. This looks good to me.
-Chris.
P.S. I will give others a chance to comment. If no objections, I will
push this tomorrow for you.
On 12/08/2013 13:33, Ivan Gerasimov wrote:
David, Chris,
I reverted back NULL-checking.
Now the change consists of one line removal and a regression test.
Webrev: http://cr.openjdk.java.net/~igerasim/8022584/6/webrev/
Hg export:
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch
Sincerely yours,
Ivan
On 09.08.2013 16:18, David Holmes wrote:
Hi Chris,
On 9/08/2013 8:36 PM, Chris Hegarty wrote:
Firstly, I think the memory leak issue should be moved forward
separately to this cleanup effort. They are unrelated, and I'm starting
to get the feeling that this could take some time to reach conclusion.
It seems reasonable to separate the issues.
I agree. I'm sure when Alan suggested to check the return he didn't
expect it to unravel like this :) As we know hotspot will never
actually return NULL there is no urgency to add this in.
On 09/08/2013 10:27, Ivan Gerasimov wrote:
Chris,
I would use this
if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
JNU_ThrowOutOfMemoryError(env, "GetStringUTFChars failed");
return NULL/JNI_False/-1;
}
If I understand it correctly, JNU_ThrowOutOfMemoryError throws an
exception only if it hasn't been already thrown.
JNU_ThrowOutOfMemoryError is simply a convenience wrapper for
JNU_ThrowByName(env, "java/lang/OutOfMemoryError", msg);
...and JNU_ThrowByName [1] is defined as...
JNU_ThrowByName(JNIEnv *env, const char *name, const char *msg) {
class cls = (*env)->FindClass(env, name);
if (cls != 0) /* Otherwise an exception has already been thrown */
(*env)->ThrowNew(env, cls, msg);
}
}
Neither FindClass or ThrowNew is safe to call if there is a pending
exception [1].
Right - we have to check for a pending exception before trying to
throw one.
Now the issue comes down to; could there ever be a pending exception if
GetStringUTFChars returns NULL? The latest specs doesn't indicate that
there could be, but every copy of "The Java Native Interface
Programmer's Guide and Specification" I can find does. There also
appears to be an assumption of this if you look at the usages in the
JDK.
AFAIK there is only one version of the JNI spec book and it never got
updated. The official spec says no throw, but when people have the
book on their bookshelf that is what they tend to rely on. I looked at
some of the usages and they seem exception agnostic - many of them
don't even check for NULL :(
The implementation as it stands will not throw and will not return NULL.
I would really like to get a definitive answer on the JNI specification
for GetStringUTFChars before making any changes here.
The JNI spec (as opposed to the book) is definitive. If we don't like
what is there then it requires a spec change.
I can't find any reference to this particular issue being raised before.
Cheers,
David
-Chris.
[1]
http://hg.openjdk.java.net/jdk8/tl/jdk/file/54f0ccdd9ad7/src/share/native/common/jni_util.c
[2]
http://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp17626
Sincerely yours,
Ivan
On 09.08.2013 11:25, Chris Hegarty wrote:
On 09/08/2013 06:47, David Holmes wrote:
Sorry I messed this up. The JNI book says GetStringUTFChars will
return
NULL and post OOME but the JNI spec (latest version 6.0) does not
- it
only says it will return NULL on failure.
This is indeed strange. Most usages of this function in the jdk expect
the former. If this is not the case, then we may need to do an audit
of all usages.
So your previous version was the more correct. Given we just
failed to
allocate C-heap I think we are on thin ice anyway, but better to at
least attempt to do the right thing.
I'm not sure what the right thing to do here is? It seems a little
unwieldy!
if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
if ((*env)->ExceptionOccurred(env)) {
return NULL/JNI_False/-1;
} else {
throwException(env, "java/lang/InternalError", "GetStringUTFChars
failed");
return NULL/JNI_False/-1;
}
Given we have no idea why GetStringUTFChars may have failed, what
exception do we throw?
Also worth noting is that this bug fix has moved away from the
original problem (memory leak), and is now focused on code cleanup.
If we cannot get agreement on the cleanup, or it looks like more
clarification is needed around the cleanup effort, then I would like
to suggest that we proceed with the original fix for the memory leak
and separate out the cleanup effort.
-Chris.
FYI I filed 8022683 to fix GetStringUTFChars.
David
-
On 9/08/2013 3:21 PM, David Holmes wrote:
Thumbs up!
Thanks,
David
On 9/08/2013 8:19 AM, Ivan Gerasimov wrote:
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 ma
Re: RFR [8022584] Memory leak in some NetworkInterface methods
Thank you Chris!
On 12.08.2013 16:43, Chris Hegarty wrote:
Thank you Ivan. This looks good to me.
-Chris.
P.S. I will give others a chance to comment. If no objections, I will
push this tomorrow for you.
On 12/08/2013 13:33, Ivan Gerasimov wrote:
David, Chris,
I reverted back NULL-checking.
Now the change consists of one line removal and a regression test.
Webrev: http://cr.openjdk.java.net/~igerasim/8022584/6/webrev/
Hg export:
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch
Sincerely yours,
Ivan
On 09.08.2013 16:18, David Holmes wrote:
Hi Chris,
On 9/08/2013 8:36 PM, Chris Hegarty wrote:
Firstly, I think the memory leak issue should be moved forward
separately to this cleanup effort. They are unrelated, and I'm
starting
to get the feeling that this could take some time to reach conclusion.
It seems reasonable to separate the issues.
I agree. I'm sure when Alan suggested to check the return he didn't
expect it to unravel like this :) As we know hotspot will never
actually return NULL there is no urgency to add this in.
On 09/08/2013 10:27, Ivan Gerasimov wrote:
Chris,
I would use this
if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
JNU_ThrowOutOfMemoryError(env, "GetStringUTFChars failed");
return NULL/JNI_False/-1;
}
If I understand it correctly, JNU_ThrowOutOfMemoryError throws an
exception only if it hasn't been already thrown.
JNU_ThrowOutOfMemoryError is simply a convenience wrapper for
JNU_ThrowByName(env, "java/lang/OutOfMemoryError", msg);
...and JNU_ThrowByName [1] is defined as...
JNU_ThrowByName(JNIEnv *env, const char *name, const char *msg) {
class cls = (*env)->FindClass(env, name);
if (cls != 0) /* Otherwise an exception has already been thrown */
(*env)->ThrowNew(env, cls, msg);
}
}
Neither FindClass or ThrowNew is safe to call if there is a pending
exception [1].
Right - we have to check for a pending exception before trying to
throw one.
Now the issue comes down to; could there ever be a pending
exception if
GetStringUTFChars returns NULL? The latest specs doesn't indicate that
there could be, but every copy of "The Java Native Interface
Programmer's Guide and Specification" I can find does. There also
appears to be an assumption of this if you look at the usages in the
JDK.
AFAIK there is only one version of the JNI spec book and it never got
updated. The official spec says no throw, but when people have the
book on their bookshelf that is what they tend to rely on. I looked at
some of the usages and they seem exception agnostic - many of them
don't even check for NULL :(
The implementation as it stands will not throw and will not return
NULL.
I would really like to get a definitive answer on the JNI
specification
for GetStringUTFChars before making any changes here.
The JNI spec (as opposed to the book) is definitive. If we don't like
what is there then it requires a spec change.
I can't find any reference to this particular issue being raised
before.
Cheers,
David
-Chris.
[1]
http://hg.openjdk.java.net/jdk8/tl/jdk/file/54f0ccdd9ad7/src/share/native/common/jni_util.c
[2]
http://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp17626
Sincerely yours,
Ivan
On 09.08.2013 11:25, Chris Hegarty wrote:
On 09/08/2013 06:47, David Holmes wrote:
Sorry I messed this up. The JNI book says GetStringUTFChars will
return
NULL and post OOME but the JNI spec (latest version 6.0) does not
- it
only says it will return NULL on failure.
This is indeed strange. Most usages of this function in the jdk
expect
the former. If this is not the case, then we may need to do an audit
of all usages.
So your previous version was the more correct. Given we just
failed to
allocate C-heap I think we are on thin ice anyway, but better to at
least attempt to do the right thing.
I'm not sure what the right thing to do here is? It seems a little
unwieldy!
if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
if ((*env)->ExceptionOccurred(env)) {
return NULL/JNI_False/-1;
} else {
throwException(env, "java/lang/InternalError", "GetStringUTFChars
failed");
return NULL/JNI_False/-1;
}
Given we have no idea why GetStringUTFChars may have failed, what
exception do we throw?
Also worth noting is that this bug fix has moved away from the
original problem (memory leak), and is now focused on code cleanup.
If we cannot get agreement on the cleanup, or it looks like more
clarification is needed around the cleanup effort, then I would like
to suggest that we proceed with the original fix for the memory leak
and separate out the cleanup effort.
-Chris.
FYI I filed 8022683 to fix GetStringUTFChars.
David
-
On 9/08/2013 3:21 PM, David Holmes wrote:
Thumbs up!
Thanks,
David
On 9/08/2013 8:19 AM, Ivan Gerasimov wrote:
Thanks David!
On 09.08.2013 1:15, David Holmes wrote:
Main fix looks good to me.
Regression test may need some twea
hg: jdk8/tl/jdk: 2 new changesets
Changeset: ffacf3e7a130 Author:mullan Date: 2013-08-12 09:03 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ffacf3e7a130 8016848: javax_security/auth/login tests fail in compact 1 and 2 profiles Summary: Change the default value of the "login.configuration.provider" security property to sun.security.provider.ConfigFile Reviewed-by: xuelei ! src/share/classes/com/sun/security/auth/login/ConfigFile.java ! src/share/classes/javax/security/auth/login/Configuration.java + src/share/classes/sun/security/provider/ConfigFile.java - src/share/classes/sun/security/provider/ConfigSpiFile.java ! src/share/classes/sun/security/provider/SunEntries.java ! src/share/lib/security/java.security-linux ! src/share/lib/security/java.security-macosx ! src/share/lib/security/java.security-solaris ! src/share/lib/security/java.security-windows Changeset: d73fbf005f5f Author:mullan Date: 2013-08-12 09:29 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d73fbf005f5f Merge - src/share/classes/java/net/package.html - test/java/lang/System/MacJNUEncoding/ExpectedEncoding.java - test/java/lang/System/MacJNUEncoding/MacJNUEncoding.sh
hg: jdk8/tl/langtools: 2 new changesets
Changeset: f7f271bd74a2 Author:mcimadamore Date: 2013-08-12 17:25 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/f7f271bd74a2 6537020: JCK tests: a compile-time error should be given in case of ambiguously imported fields (types, methods) Summary: Hiding check does not support interface multiple inheritance Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/code/Scope.java ! src/share/classes/com/sun/tools/javac/code/Symbol.java ! src/share/classes/com/sun/tools/javac/comp/Check.java ! src/share/classes/com/sun/tools/javac/comp/MemberEnter.java ! src/share/classes/com/sun/tools/javac/comp/Resolve.java ! test/tools/javac/4980495/static/Test.out ! test/tools/javac/diags/examples/AlreadyDefinedStaticImport/AlreadDefinedStaticImport.java ! test/tools/javac/diags/examples/AlreadyDefinedStaticImport/p/E1.java ! test/tools/javac/diags/examples/AlreadyDefinedStaticImport/p/E2.java + test/tools/javac/staticImport/6537020/T6537020.java + test/tools/javac/staticImport/6537020/T6537020.out Changeset: af80273f630a Author:mcimadamore Date: 2013-08-12 17:28 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/af80273f630a 8021567: Javac doesn't report \"java: reference to method is ambiguous\" any more Summary: Javac incorrectly forgets about constant folding results within lambdas Reviewed-by: jjg, vromero ! src/share/classes/com/sun/tools/javac/code/Type.java ! src/share/classes/com/sun/tools/javac/comp/Attr.java + test/tools/javac/lambda/8021567/T8021567.java + test/tools/javac/lambda/8021567/T8021567.out + test/tools/javac/lambda/8021567/T8021567b.java
Re: RFR [8022584] Memory leak in some NetworkInterface methods
On 09/08/2013 13:18, David Holmes wrote: I agree. I'm sure when Alan suggested to check the return he didn't expect it to unravel like this :) As we know hotspot will never actually return NULL there is no urgency to add this in. Sorry about this, I wasn't aware of the issue in the JNI spec, we should follow up on that. Ivan's latest webrev (which limits the change to just removing the GetStringUTFChars is fine). The test is okay but it might need time to bed down. -Alan
hg: jdk8/tl/jdk: 8015780: java/lang/reflect/Method/GenericStringTest.java failing
Changeset: 70c8f4a4b8d6 Author:vromero Date: 2013-08-12 17:40 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/70c8f4a4b8d6 8015780: java/lang/reflect/Method/GenericStringTest.java failing Reviewed-by: darcy, jfranck ! test/ProblemList.txt ! test/java/lang/reflect/Method/GenericStringTest.java
hg: jdk8/tl/jdk: 8022753: SQLXML javadoc example typo
Changeset: cc64a05836a7 Author:lancea Date: 2013-08-12 16:09 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/cc64a05836a7 8022753: SQLXML javadoc example typo Reviewed-by: alanb, mchung ! src/share/classes/java/sql/SQLXML.java
Re: RFR [8022584] Memory leak in some NetworkInterface methods
Thanks Ivan.
David
On 12/08/2013 10:33 PM, Ivan Gerasimov wrote:
David, Chris,
I reverted back NULL-checking.
Now the change consists of one line removal and a regression test.
Webrev: http://cr.openjdk.java.net/~igerasim/8022584/6/webrev/
Hg export:
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch
Sincerely yours,
Ivan
On 09.08.2013 16:18, David Holmes wrote:
Hi Chris,
On 9/08/2013 8:36 PM, Chris Hegarty wrote:
Firstly, I think the memory leak issue should be moved forward
separately to this cleanup effort. They are unrelated, and I'm starting
to get the feeling that this could take some time to reach conclusion.
It seems reasonable to separate the issues.
I agree. I'm sure when Alan suggested to check the return he didn't
expect it to unravel like this :) As we know hotspot will never
actually return NULL there is no urgency to add this in.
On 09/08/2013 10:27, Ivan Gerasimov wrote:
Chris,
I would use this
if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
JNU_ThrowOutOfMemoryError(env, "GetStringUTFChars failed");
return NULL/JNI_False/-1;
}
If I understand it correctly, JNU_ThrowOutOfMemoryError throws an
exception only if it hasn't been already thrown.
JNU_ThrowOutOfMemoryError is simply a convenience wrapper for
JNU_ThrowByName(env, "java/lang/OutOfMemoryError", msg);
...and JNU_ThrowByName [1] is defined as...
JNU_ThrowByName(JNIEnv *env, const char *name, const char *msg) {
class cls = (*env)->FindClass(env, name);
if (cls != 0) /* Otherwise an exception has already been thrown */
(*env)->ThrowNew(env, cls, msg);
}
}
Neither FindClass or ThrowNew is safe to call if there is a pending
exception [1].
Right - we have to check for a pending exception before trying to
throw one.
Now the issue comes down to; could there ever be a pending exception if
GetStringUTFChars returns NULL? The latest specs doesn't indicate that
there could be, but every copy of "The Java Native Interface
Programmer's Guide and Specification" I can find does. There also
appears to be an assumption of this if you look at the usages in the
JDK.
AFAIK there is only one version of the JNI spec book and it never got
updated. The official spec says no throw, but when people have the
book on their bookshelf that is what they tend to rely on. I looked at
some of the usages and they seem exception agnostic - many of them
don't even check for NULL :(
The implementation as it stands will not throw and will not return NULL.
I would really like to get a definitive answer on the JNI specification
for GetStringUTFChars before making any changes here.
The JNI spec (as opposed to the book) is definitive. If we don't like
what is there then it requires a spec change.
I can't find any reference to this particular issue being raised before.
Cheers,
David
-Chris.
[1]
http://hg.openjdk.java.net/jdk8/tl/jdk/file/54f0ccdd9ad7/src/share/native/common/jni_util.c
[2]
http://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp17626
Sincerely yours,
Ivan
On 09.08.2013 11:25, Chris Hegarty wrote:
On 09/08/2013 06:47, David Holmes wrote:
Sorry I messed this up. The JNI book says GetStringUTFChars will
return
NULL and post OOME but the JNI spec (latest version 6.0) does not
- it
only says it will return NULL on failure.
This is indeed strange. Most usages of this function in the jdk expect
the former. If this is not the case, then we may need to do an audit
of all usages.
So your previous version was the more correct. Given we just
failed to
allocate C-heap I think we are on thin ice anyway, but better to at
least attempt to do the right thing.
I'm not sure what the right thing to do here is? It seems a little
unwieldy!
if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
NULL) {
if ((*env)->ExceptionOccurred(env)) {
return NULL/JNI_False/-1;
} else {
throwException(env, "java/lang/InternalError", "GetStringUTFChars
failed");
return NULL/JNI_False/-1;
}
Given we have no idea why GetStringUTFChars may have failed, what
exception do we throw?
Also worth noting is that this bug fix has moved away from the
original problem (memory leak), and is now focused on code cleanup.
If we cannot get agreement on the cleanup, or it looks like more
clarification is needed around the cleanup effort, then I would like
to suggest that we proceed with the original fix for the memory leak
and separate out the cleanup effort.
-Chris.
FYI I filed 8022683 to fix GetStringUTFChars.
David
-
On 9/08/2013 3:21 PM, David Holmes wrote:
Thumbs up!
Thanks,
David
On 9/08/2013 8:19 AM, Ivan Gerasimov wrote:
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 t
