Re: [15] RFR 8241760 : Typos: empty lines in javadoc, inconsistent indents, etc. (net and nio)
Thank you guy for your reviews! Pushed. On 3/30/20 5:27 AM, Daniel Fuchs wrote: Looks good to me Ivan. Thanks for this cleanup! best regards, -- daniel On 30/03/2020 03:34, Ivan Gerasimov wrote: Thank you Alan and Pavel! My apologies for a wrong link in the initial request. Here's the new webrev with the changes suggested by Pavel: http://cr.openjdk.java.net/~igerasim/8241760/01/webrev/ With kind regards, Ivan G. -- With kind regards, Ivan Gerasimov
Re: [15] RFR 8241760 : Typos: empty lines in javadoc, inconsistent indents, etc. (net and nio)
Thank you Alan and Pavel! My apologies for a wrong link in the initial request. Here's the new webrev with the changes suggested by Pavel: http://cr.openjdk.java.net/~igerasim/8241760/01/webrev/ With kind regards, Ivan G. On 3/29/20 2:43 PM, Pavel Rappo wrote: Ivan, 1. ByteBuffered has an awkwardly looking top-level doc comment markup/formatting. The "payload" begins on the same line as the /** marker. Also, the tag is weirdly placed. Since you have fixed similar issues already (URLConnection), you could probably do the same here. Your call. 2. I know you probably wanted to confine this change to just the formatting, but... maybe we could fix the "protocol handers" typo as an exception? Other than that, the changes look good. Thanks for doing this! -Pavel On 29 Mar 2020, at 05:44, Ivan Gerasimov wrote: Hello! The fix follows up on JDK-8241727 [1]. This is a javadoc/comments only fix in the net and nio areas. The changes are to remove redundant empty lines, correct indentation, or otherwise restore harmony. Would you please help review this rather technical fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8241727 WEBREV: http://cr.openjdk.java.net/~igerasim/8241727/00/webrev/ Thank in advance! [1] https://bugs.openjdk.java.net/browse/JDK-8241727 -- With kind regards, Ivan Gerasimov -- With kind regards, Ivan Gerasimov
[15] RFR 8241760 : Typos: empty lines in javadoc, inconsistent indents, etc. (net and nio)
Hello! The fix follows up on JDK-8241727 [1]. This is a javadoc/comments only fix in the net and nio areas. The changes are to remove redundant empty lines, correct indentation, or otherwise restore harmony. Would you please help review this rather technical fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8241727 WEBREV: http://cr.openjdk.java.net/~igerasim/8241727/00/webrev/ Thank in advance! [1] https://bugs.openjdk.java.net/browse/JDK-8241727 -- With kind regards, Ivan Gerasimov
Re: [14-dev] RFR : (dc) MulticastSendReceiveTests.java failing with ENOMEM when joining group (OS X 10.9)
Thanks everyone for reviewing! On 3/20/20 5:39 AM, Alan Bateman wrote: On 20/03/2020 12:22, Daniel Fuchs wrote: Hi Ivan, Looks good to me. I wonder if you should add @bug 8044365 to the MulticastSendReceiveTests.java while you're at it. Every test that joins a multicast has the potential of hitting this, it's very random, so probably not useful to put it on this specific test. I think we need to put time into writing a native reproducer for Apple and get a bug submitted. Right. The fix is not relevant to a particular test, so I'll set the noreg-hard label. Recently I've hit it with a non-test application. With kind regards, Ivan
[14-dev] RFR : (dc) MulticastSendReceiveTests.java failing with ENOMEM when joining group (OS X 10.9)
Hello! An OOM error is intermittently observed on MacOS when joining a multicast group. A workaround was implemented as part of the fix for JDK-8236925. It is proposed to backport a portion of that fix, which helps to mitigate the issue on MacOS. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8044365 WEBREV: http://cr.openjdk.java.net/~igerasim/8044365/00/webrev/ Mach5 control build in progress. -- With kind regards, Ivan Gerasimov
RFR 8233886 : TEST_BUG jdk/java/net/CookieHandler/B6791927.java hit hardcoded expiration date
Hello! This test started to fail recently. This is because it has a cookie expiration date hardcoded as 'Sat, 09-Nov-2019 23:12:40 GMT'. The simplest fix is to move the date forward. I also made this test to run in the othervm mode because it modifies the default locale. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8233886 WEBREV: http://cr.openjdk.java.net/~igerasim/8233886/00/webrev/ -- With kind regards, Ivan Gerasimov
Re: RFR (S) 8230407 : SocketPermission and FilePermission action list allows leading comma
Hi Chris! On 10/3/19 8:05 AM, Chris Hegarty wrote: Ivan, On 3 Oct 2019, at 04:41, Ivan Gerasimov wrote: ... So, I filed CSR: https://bugs.openjdk.java.net/browse/JDK-8231805 to cover the addition of @throws paragraph in the javadoc of SocketPermission. I would really appreciate it, if someone helped to review it. Since we’re here ... ;-) It would be good to specify the NPE behavior of the constructor. Here are the changes for SocketPermission. If you agree, fold them into your patch and CSR. ( I’ve included test changes to verify the new tighter spec ) https://cr.openjdk.java.net/~chegar/8230407.extra/ Yes, it's a good point, thanks! I've adopted your suggested changes and the test: http://cr.openjdk.java.net/~igerasim/8230407/02/webrev/ CSR was also updated accordingly: https://bugs.openjdk.java.net/browse/JDK-8231805 With kind regards, Ivan -Chris. -- With kind regards, Ivan Gerasimov
Re: RFR (S) 8230407 : SocketPermission and FilePermission action list allows leading comma
Thanks Peter, your variant looks cute!
Let's keep this code in mind as a candidate for refactoring once the
switch expression will make its way to the standard.
I would hesitate to allow using experimental features in building the
JDK just for that code :)
With kind regards,
Ivan
On 10/2/19 11:40 PM, Peter Levart wrote:
Hi Ivan,
On 10/1/19 10:26 PM, Ivan Gerasimov wrote:
Hello!
The constructors of SocketPermission and FilePermission expect a
String argument with comma-separated list of actions.
If the list is malformed, then the constructors throw
IllegalArgumentException.
It turns out that the current implementation fails to throw IAE if
the list starts with a leading comma.
Would you please help review a simple fix, which will make the
behavior more consistent?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8230407
WEBREV: http://cr.openjdk.java.net/~igerasim/8230407/00/webrev/
With new switch expressions the logic could be a little clearer. For
example:
for (boolean seencomma = false; i >= matchlen &&
!seencomma; i--) {
seencomma = switch (a[i - matchlen]) {
case ' ', '\r', '\n', '\f', '\t': yield false;
case ',': if (i > matchlen) yield true;
default: throw new IllegalArgumentException(
"invalid permission: " + actions);
};
}
This is still experimental, but I think it will be proposed to be
standard soon.
If you want to backport it later, then maybe you don't want to do that
now.
Regards, Peter
--
With kind regards,
Ivan Gerasimov
Re: RFR (S) 8230407 : SocketPermission and FilePermission action list allows leading comma
Thank you Joe for checking it! On 10/2/19 4:38 PM, Joe Darcy wrote: Hello, At least from a quick reading, either the spec change or the behavior change would seem to merit a CSR. Sigh. I was hopping it'll be a quick fix :-) So, I filed CSR: https://bugs.openjdk.java.net/browse/JDK-8231805 to cover the addition of @throws paragraph in the javadoc of SocketPermission. I would really appreciate it, if someone helped to review it. W.r.t the behavior change, I don't think the fix has to be counted as such. Current implementation already would throw IllegalArgumentException if the action list were malformed (for example if it were " , accept", or "connect,,accept", or "connect,", etc.) The only case when it would *not* throw IAE is when the argument *immediately* starts with a comma, and that's what the fix is about. It's not like if we used to allow commas in arbitrary places and stopped doing that. Instead, it just turned out that the code fails to catch one specific pattern of malformed action list. With kind regards, Ivan Cheers, -Joe On 10/2/2019 4:26 PM, Ivan Gerasimov wrote: Hi Chris! Thank you very much for review! I agree that it makes sense to update the javadoc for consistency. I don't think CSR is required in this case, is it? (IAE is unchecked anyway, and the fix doesn't really change the behavior.) Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8230407/01/webrev/ With kind regards, Ivan On 10/2/19 6:44 AM, Chris Hegarty wrote: Ivan, On 01/10/2019 21:26, Ivan Gerasimov wrote: Hello! The constructors of SocketPermission and FilePermission expect a String argument with comma-separated list of actions. If the list is malformed, then the constructors throw IllegalArgumentException. It turns out that the current implementation fails to throw IAE if the list starts with a leading comma. Would you please help review a simple fix, which will make the behavior more consistent? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8230407 WEBREV: http://cr.openjdk.java.net/~igerasim/8230407/00/webrev/ The implementation changes look ok. The SocketPermission constructor should be updated to specify IAE too, right? -Chris. -- With kind regards, Ivan Gerasimov
Re: RFR (S) 8230407 : SocketPermission and FilePermission action list allows leading comma
Hi Chris! Thank you very much for review! I agree that it makes sense to update the javadoc for consistency. I don't think CSR is required in this case, is it? (IAE is unchecked anyway, and the fix doesn't really change the behavior.) Here's the updated webrev: http://cr.openjdk.java.net/~igerasim/8230407/01/webrev/ With kind regards, Ivan On 10/2/19 6:44 AM, Chris Hegarty wrote: Ivan, On 01/10/2019 21:26, Ivan Gerasimov wrote: Hello! The constructors of SocketPermission and FilePermission expect a String argument with comma-separated list of actions. If the list is malformed, then the constructors throw IllegalArgumentException. It turns out that the current implementation fails to throw IAE if the list starts with a leading comma. Would you please help review a simple fix, which will make the behavior more consistent? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8230407 WEBREV: http://cr.openjdk.java.net/~igerasim/8230407/00/webrev/ The implementation changes look ok. The SocketPermission constructor should be updated to specify IAE too, right? -Chris. -- With kind regards, Ivan Gerasimov
Re: RFR (XS) 8230415 : Avoid redundant permission checking in FilePermissionCollection and SocketPermissionCollection
Thank you Sean for reviewing!
With kind regards,
Ivan
On 9/27/19 7:20 AM, Sean Mullan wrote:
Hi Ivan,
The fix looks good. Good catch.
--Sean
On 8/30/19 7:32 PM, Ivan Gerasimov wrote:
Hello!
In the two implementations of
PermissionCollection.implies(Permission), all the permissions are
traversed, and their corresponding bit mask are checked.
For example, here's a snippet from FilePermission.java:
int desired = fperm.getMask();
int effective = 0;
int needed = desired;
for (Permission perm : perms.values()) {
FilePermission fp = (FilePermission)perm;
if (((needed & fp.getMask()) != 0) &&
fp.impliesIgnoreMask(fperm)) {
effective |= fp.getMask();
if ((effective & desired) == desired) {
return true;
}
needed = (desired ^ effective);// <<< should be
(desired & ~effective)
}
}
Here, if a permission's mask `fp.getMask()` intersects with `needed`,
but does not fully cover all the needed bits, the variable `needed`
is updated as XOR of desired and effective. This can raise a
not-really-needed bits in the `needed` mask, so that for all
subsequent permissions from the collection with that unneeded bits in
the mask, an expensive fp.impliesIgnoreMask(fperm) will be executed.
The fix does not change the behavior, but helps avoid unnecessary
calls to impliesIgnoreMask().
Would you please help review a trivial fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8230415
WEBREV: http://cr.openjdk.java.net/~igerasim/8230415/00/webrev/
Thanks in advance!
--
With kind regards,
Ivan Gerasimov
Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n
On 6/12/19 10:02 AM, Brian Burkhalter wrote:
Actually, never mind, I am being completely lame here: both
NET_ThrowNew() and the Windows function LocalFree() are robust to a
NULL-valued buf so I think we can just remove the n > 0 or buf == NULL
check altogether.
That's true, assuming that you initialize buf = NULL and hopping that
FormatMessage won't change buf upon failure.
I wish MSDN were a little bit more specific here.
I am fine with
1)
362 TCHAR *buf = NULL;
2)
unconditional
395NET_ThrowNew(env, err, buf);
396LocalFree(buf);
With kind regards,
Ivan
Sorry for the noise: I should have checked this first.
Thanks,
Brian
On Jun 12, 2019, at 9:51 AM, Brian Burkhalter
mailto:brian.burkhal...@oracle.com>> wrote:
I am perhaps beating a dead horse here, but how about this instead?
if (n > 0) {
NET_ThrowNew(env, err, buf);
LocalFree(buf);
} else {
NET_ThrowNew(env, err, "FormatMessage failed");
}
After all, an error *did* occur.
--
With kind regards,
Ivan Gerasimov
Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n
Thanks Brian!
The webrev looks fine to me.
I think that *most likely* the check if (buf != NULL) will work as
expected: buf will only be set to non-NULL value upon success.
However, the documentation for the function FormatMessage states:
"""
If the function fails, the return value is zero.
"""
So, my preference would be if (n > 0).
With kind regards,
Ivan
On 6/11/19 5:26 PM, Brian Burkhalter wrote:
Hi Ivan,
I updated the patch:
http://cr.openjdk.java.net/~bpb/8223813/webrev.01/
<http://cr.openjdk.java.net/%7Ebpb/8223813/webrev.01/>
Please see comments inline below.
On Jun 11, 2019, at 5:06 PM, Ivan Gerasimov
mailto:ivan.gerasi...@oracle.com>> wrote:
Inet4AddressImpl.c:
1) There is an extra space after FormatMessage,
Fixed.
2) It is preexisting. The code doesn't check if FormatMessage failed
to allocate the buffer.
It's not clear from the MSDN documentation, if the pointer to the
buffer will be set to NULL upon the failure.
If it does not, then subsequent NET_ThrowNew(env, err, buf);
LocalFree(buf); may hit uninitialized memory.
It would be more accurate to invoke them only if (n > 0).
I put “if (buf != NULL)” instead of “if (n > 0)”.
3) It purely optional, but you may want to use the TEXT macro to
append the L prefix to the character literals, if TCHAR is defined to
be WCHAR:
389 if (buf[n - 1] == TEXT('\n')) n--;
390 if (buf[n - 1] == TEXT('\r')) n--;
391 if (buf[n - 1] == TEXT('.')) n--;
392 buf[n] = TEXT('\0');
It may make the compiler just a tiny bit happier :)
So changed.
Everything else looks good to me.
Thanks for the comments!
Brian
--
With kind regards,
Ivan Gerasimov
Re: 8223813: (aio) Iocp.getErrorMessage should drop trailing \r\n
Hi Brian!
Inet4AddressImpl.c:
1) There is an extra space after FormatMessage,
2) It is preexisting. The code doesn't check if FormatMessage failed to
allocate the buffer.
It's not clear from the MSDN documentation, if the pointer to the buffer
will be set to NULL upon the failure.
If it does not, then subsequent NET_ThrowNew(env, err, buf);
LocalFree(buf); may hit uninitialized memory.
It would be more accurate to invoke them only if (n > 0).
3) It purely optional, but you may want to use the TEXT macro to append
the L prefix to the character literals, if TCHAR is defined to be WCHAR:
389 if (buf[n - 1] == TEXT('\n')) n--;
390 if (buf[n - 1] == TEXT('\r')) n--;
391 if (buf[n - 1] == TEXT('.')) n--;
392 buf[n] = TEXT('\0');
It may make the compiler just a tiny bit happier :)
Everything else looks good to me.
With kind regards,
Ivan
On 6/11/19 10:56 AM, Brian Burkhalter wrote:
https://bugs.openjdk.java.net/browse/JDK-8223813
http://cr.openjdk.java.net/~bpb/8223813/webrev.00/
<http://cr.openjdk.java.net/%7Ebpb/8223813/webrev.00/>
FormatMessage() and FormatMessageW() occur in a number of locations:
src/java.base/windows/native/libjli/java_md.c
src/java.base/windows/native/libnet/Inet4AddressImpl.c
src/java.base/windows/native/libjava/ProcessImpl_md.c
src/java.base/windows/native/libjava/jni_util_md.c
src/java.base/windows/native/libnio/ch/Iocp.c
src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c
src/java.base/share/native/libzip/zlib/gzlib.c
Some of these already strip the terminal CRLF (or dot + CRLF) of the
string populated by FormatMessage[W](). This patch would add removing
them, if present, from
src/java.base/windows/native/libnet/Inet4AddressImpl.c
src/java.base/windows/native/libnio/ch/Iocp.c
src/java.base/windows/native/libnio/fs/WindowsNativeDispatcher.c
One question is whether it would be better just to consolidate this
code into two methods for example in jni_uitl and call the methods
from the other locations. There are already getLastErrorString() and
getErrorString() for chars here.
Also, I am not sure how to test this effectively. The code passes all
tiers as-is.
Thanks,
Brian
--
With kind regards,
Ivan Gerasimov
Re: RFR 8170494 : JNI exception pending in PlainDatagramSocketImpl.c
Thanks Vyom!
On 3/20/19 10:13 PM, Vyom Tiwari wrote:
Hi Ivan,
Looks OK to me, in case of exception "ni" will be null you can use
the macro(CHECK_NULL_RETURN) if you are not clearing the JNI
exception, this will save at least one additional function(
"(*env)->ExceptionOccurred(env)" ) call.
Java_java_net_NetworkInterface_getByInetAddress0 may return NULL if
there were no interfaces found.
We should not return from getMulticastInterface() in this case.
With kind regards,
Ivan
Thanks,
Vyom
On Wed, Mar 20, 2019 at 9:49 PM Ivan Gerasimov
mailto:ivan.gerasi...@oracle.com>> wrote:
Hello!
The function Java_java_net_NetworkInterface_getByInetAddress0 may
throw,
so after calling it we need to check if an exception is pending.
Would you please help review a one-line fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8170494
WEBREV: http://cr.openjdk.java.net/~igerasim/8170494/00/webrev/
<http://cr.openjdk.java.net/%7Eigerasim/8170494/00/webrev/>
Thanks in advance!
--
With kind regards,
Ivan Gerasimov
--
Thanks,
Vyom
--
With kind regards,
Ivan Gerasimov
RFR 8170494 : JNI exception pending in PlainDatagramSocketImpl.c
Hello! The function Java_java_net_NetworkInterface_getByInetAddress0 may throw, so after calling it we need to check if an exception is pending. Would you please help review a one-line fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8170494 WEBREV: http://cr.openjdk.java.net/~igerasim/8170494/00/webrev/ Thanks in advance! -- With kind regards, Ivan Gerasimov
Re: RFR 8007606 : Handle realloc() failure in unix/native/libnet/net_util_md.c correctly
Thank you Christoph and Mattias!
I've filed
https://bugs.openjdk.java.net/browse/JDK-8217291 and
https://bugs.openjdk.java.net/browse/JDK-8217292
to handle the realloc() issues in other areas.
With kind regards,
Ivan
On 1/16/19 2:22 AM, Baesken, Matthias wrote:
Hi Ivan ,looks good to me too (not a Reviewer however).
Do you think we should address the other reallocs with unhandeled return code
?
Best regards, Matthias
-Original Message-
From: Langer, Christoph
Sent: Dienstag, 15. Januar 2019 20:58
To: Ivan Gerasimov ; Baesken, Matthias
; net-dev@openjdk.java.net
Subject: RE: RFR 8007606 : Handle realloc() failure in
unix/native/libnet/net_util_md.c correctly
Hi Ivan,
yes, sure, push it
Best regards
Christoph
-Original Message-
From: Ivan Gerasimov
Sent: Dienstag, 15. Januar 2019 20:53
To: Baesken, Matthias ; net-
d...@openjdk.java.net; Langer, Christoph
Subject: Re: RFR 8007606 : Handle realloc() failure in
unix/native/libnet/net_util_md.c correctly
Hello!
Do you think it is good to go now?
With kind regards,
Ivan
On 1/11/19 11:30 AM, Ivan Gerasimov wrote:
Good catch, thank you!
Indeed, if we don't reset localifsSize then we could end up accessing
already freed memory, which is worse than just a memory leak.
Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8007606/01/webrev/
With kind regards,
Ivan
On 1/11/19 4:43 AM, Baesken, Matthias wrote:
Hi Ivan,
Shouldn't you resetlocalifsSize to 0 in case of the early
return ? The comment says localifsSize is the size of the array so
the size of the array is 0 again after freeing.
637 static struct localinterface *localifs = 0;
638 static int localifsSize = 0;/* size of array */
639 static int nifs = 0;/* number of entries used in
array */
...
679 if (localifsTemp == 0) {
680 free(localifs);
681 localifs = 0;
682 nifs = 0;
683 fclose(f);
684 return;
685 }
Best regards, Matthias
Date: Thu, 10 Jan 2019 20:29:08 -0800
From: Ivan Gerasimov
To: "net-dev@openjdk.java.net"
Subject: RFR 8007606 : Handle realloc() failure in
unix/native/libnet/net_util_md.c correctly
Message-ID: <3dc3c26b-fea7-2538-2c7a-bfa623f2f...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Hello!
This seems to be the last use of realloc() without proper handling of a
failure.
Would you please help review a trivial fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8007606
WEBREV: http://cr.openjdk.java.net/~igerasim/8007606/00/webrev/
Thanks in advance!
--
With kind regards,
Ivan Gerasimov
--
With kind regards,
Ivan Gerasimov
--
With kind regards,
Ivan Gerasimov
Re: RFR 8007606 : Handle realloc() failure in unix/native/libnet/net_util_md.c correctly
Hello!
Do you think it is good to go now?
With kind regards,
Ivan
On 1/11/19 11:30 AM, Ivan Gerasimov wrote:
Good catch, thank you!
Indeed, if we don't reset localifsSize then we could end up accessing
already freed memory, which is worse than just a memory leak.
Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8007606/01/webrev/
With kind regards,
Ivan
On 1/11/19 4:43 AM, Baesken, Matthias wrote:
Hi Ivan,
Shouldn't you resetlocalifsSize to 0 in case of the early
return ? The comment says localifsSize is the size of the array so
the size of the array is 0 again after freeing.
637 static struct localinterface *localifs = 0;
638 static int localifsSize = 0;/* size of array */
639 static int nifs = 0;/* number of entries used in
array */
...
679 if (localifsTemp == 0) {
680 free(localifs);
681 localifs = 0;
682 nifs = 0;
683 fclose(f);
684 return;
685 }
Best regards, Matthias
Date: Thu, 10 Jan 2019 20:29:08 -0800
From: Ivan Gerasimov
To: "net-dev@openjdk.java.net"
Subject: RFR 8007606 : Handle realloc() failure in
unix/native/libnet/net_util_md.c correctly
Message-ID: <3dc3c26b-fea7-2538-2c7a-bfa623f2f...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Hello!
This seems to be the last use of realloc() without proper handling of a
failure.
Would you please help review a trivial fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8007606
WEBREV: http://cr.openjdk.java.net/~igerasim/8007606/00/webrev/
Thanks in advance!
--
With kind regards,
Ivan Gerasimov
--
With kind regards,
Ivan Gerasimov
Re: RFR 8007606 : Handle realloc() failure in unix/native/libnet/net_util_md.c correctly
Thank you Christoph!
Mixed code formatting style is used in this file. There are too many
places where an extra space is put after a function name.
I think it's better to only fix the style on the already touched lines
to avoid blurring the fix.
With kind regards,
Ivan
On 1/11/19 9:04 AM, Langer, Christoph wrote:
Hi,
that's right, good catch. Either set localifs to 0 or maybe even keep the old
pointer with the old value of localifs. I guess the case is a bit theoretical
but it should be done right.
In line 695 fclose (f); the formatting can be fixed (also remove space
between fclose and the bracket).
Thanks
Christoph
-Original Message-
From: net-dev On Behalf Of
Baesken, Matthias
Sent: Freitag, 11. Januar 2019 13:43
To: net-dev@openjdk.java.net
Subject: [CAUTION] Re: RFR 8007606 : Handle realloc() failure in
unix/native/libnet/net_util_md.c correctly
Hi Ivan,
Shouldn't you resetlocalifsSize to 0 in case of the early return ? The
comment says localifsSize is the size of the array so the size of the array is 0
again after freeing.
637 static struct localinterface *localifs = 0;
638 static int localifsSize = 0;/* size of array */
639 static int nifs = 0;/* number of entries used in array */
...
679 if (localifsTemp == 0) {
680 free(localifs);
681 localifs = 0;
682 nifs = 0;
683 fclose(f);
684 return;
685 }
Best regards, Matthias
Date: Thu, 10 Jan 2019 20:29:08 -0800
From: Ivan Gerasimov
To: "net-dev@openjdk.java.net"
Subject: RFR 8007606 : Handle realloc() failure in
unix/native/libnet/net_util_md.c correctly
Message-ID: <3dc3c26b-fea7-2538-2c7a-bfa623f2f...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Hello!
This seems to be the last use of realloc() without proper handling of a
failure.
Would you please help review a trivial fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8007606
WEBREV: http://cr.openjdk.java.net/~igerasim/8007606/00/webrev/
Thanks in advance!
--
With kind regards,
Ivan Gerasimov
--
With kind regards,
Ivan Gerasimov
Re: RFR 8007606 : Handle realloc() failure in unix/native/libnet/net_util_md.c correctly
Thank you Christoph!
Mixed code formatting style is used in this file. There are too many
places where an extra space is put after a function name.
I think it's better to only fix the style on the already touched lines
to avoid blurring the fix.
With kind regards,
Ivan
On 1/11/19 9:04 AM, Langer, Christoph wrote:
Hi,
that's right, good catch. Either set localifs to 0 or maybe even keep the old
pointer with the old value of localifs. I guess the case is a bit theoretical
but it should be done right.
In line 695 fclose (f); the formatting can be fixed (also remove space
between fclose and the bracket).
Thanks
Christoph
-Original Message-
From: net-dev On Behalf Of
Baesken, Matthias
Sent: Freitag, 11. Januar 2019 13:43
To: net-dev@openjdk.java.net
Subject: [CAUTION] Re: RFR 8007606 : Handle realloc() failure in
unix/native/libnet/net_util_md.c correctly
Hi Ivan,
Shouldn't you resetlocalifsSize to 0 in case of the early return ? The
comment says localifsSize is the size of the array so the size of the array is 0
again after freeing.
637 static struct localinterface *localifs = 0;
638 static int localifsSize = 0;/* size of array */
639 static int nifs = 0;/* number of entries used in array */
...
679 if (localifsTemp == 0) {
680 free(localifs);
681 localifs = 0;
682 nifs = 0;
683 fclose(f);
684 return;
685 }
Best regards, Matthias
Date: Thu, 10 Jan 2019 20:29:08 -0800
From: Ivan Gerasimov
To: "net-dev@openjdk.java.net"
Subject: RFR 8007606 : Handle realloc() failure in
unix/native/libnet/net_util_md.c correctly
Message-ID: <3dc3c26b-fea7-2538-2c7a-bfa623f2f...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Hello!
This seems to be the last use of realloc() without proper handling of a
failure.
Would you please help review a trivial fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8007606
WEBREV: http://cr.openjdk.java.net/~igerasim/8007606/00/webrev/
Thanks in advance!
--
With kind regards,
Ivan Gerasimov
--
With kind regards,
Ivan Gerasimov
Re: RFR 8007606 : Handle realloc() failure in unix/native/libnet/net_util_md.c correctly
Good catch, thank you!
Indeed, if we don't reset localifsSize then we could end up accessing
already freed memory, which is worse than just a memory leak.
Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8007606/01/webrev/
With kind regards,
Ivan
On 1/11/19 4:43 AM, Baesken, Matthias wrote:
Hi Ivan,
Shouldn't you resetlocalifsSize to 0 in case of the early return ? The
comment says localifsSize is the size of the array so the size of the array is
0 again after freeing.
637 static struct localinterface *localifs = 0;
638 static int localifsSize = 0;/* size of array */
639 static int nifs = 0;/* number of entries used in array */
...
679 if (localifsTemp == 0) {
680 free(localifs);
681 localifs = 0;
682 nifs = 0;
683 fclose(f);
684 return;
685 }
Best regards, Matthias
Date: Thu, 10 Jan 2019 20:29:08 -0800
From: Ivan Gerasimov
To: "net-dev@openjdk.java.net"
Subject: RFR 8007606 : Handle realloc() failure in
unix/native/libnet/net_util_md.c correctly
Message-ID: <3dc3c26b-fea7-2538-2c7a-bfa623f2f...@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Hello!
This seems to be the last use of realloc() without proper handling of a
failure.
Would you please help review a trivial fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8007606
WEBREV: http://cr.openjdk.java.net/~igerasim/8007606/00/webrev/
Thanks in advance!
--
With kind regards,
Ivan Gerasimov
--
With kind regards,
Ivan Gerasimov
RFR 8007606 : Handle realloc() failure in unix/native/libnet/net_util_md.c correctly
Hello! This seems to be the last use of realloc() without proper handling of a failure. Would you please help review a trivial fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8007606 WEBREV: http://cr.openjdk.java.net/~igerasim/8007606/00/webrev/ Thanks in advance! -- With kind regards, Ivan Gerasimov
Re: RFR [12] 8213490: Networking area nano cleanup
Also, in
src/java.base/windows/classes/sun/net/dns/ResolverConfigurationImpl.java
-// Addreses have changed
+// Addresses have changed
Wasn't it meant to be singular? I.e. "Address has changed"
And the same thing on the line 84.
src/java.base/windows/native/libnet/net_util_md.c
+ * 2. If the requested port is 0 (ie. any port) then we try to bind in
v4 space
I still see `ie.`.
Not like it really matters in this context :)
With kind regards,
Ivan
On 11/13/18 2:29 PM, Ivan Gerasimov wrote:
Thanks Pavel, looks good!
*src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java*
I wonder if 'I' needs to be capitalized, as it starts a sentence:
* Authenticator for a particular realm is single threaded.
- * ie. if multiple threads need to get credentials from the user
+ * i.e. if multiple threads need to get credentials from the user
* at the same time, then all but the first will block until
No need for a separate review, if you agree to change i -> I.
With kind regards,
Ivan
On 11/12/18 6:45 PM, Pavel Rappo wrote:
On 13 Nov 2018, at 00:35, Ivan Gerasimov wrote:
Do you want to change ie. -> i.e. here as well:
src/java.base/windows/native/libnet/net_util_md.c
- * 2. If the reqeusted port is 0 (ie. any port) then we try to bind in v4 space
+ * 2. If the requested port is 0 (ie. any port) then we try to bind in v4 space
Thanks. I have addressed "eg." too, please see the result here:
http://cr.openjdk.java.net/~prappo/8213490/webrev.02/
And a couple more of duplicate words to remove:
jdk/internal/net/http/Http1AsyncReceiver.java:// If the queue is not
empty, wait until it it is empty before
jdk/internal/net/http/Http2Connection.java:// if true, the the stream may
be assigned to this connection
After I have noticed this this pattern [*] I wrote a tiny tool to help me search
for occurrences of it in comments and javadocs. It's just a little bit more
complicated than a bunch of regexes, as javadoc structures (e.g. "{@link method
method}" or "@param number number", etc.) would otherwise yield too many false
positives. Your comment reminded me that I forgot about single-line comments.
The tool has been updated. Thanks.
[*] Pun intended
--
With kind regards,
Ivan Gerasimov
--
With kind regards,
Ivan Gerasimov
Re: RFR [12] 8213490: Networking area nano cleanup
Thanks Pavel, looks good!
*src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java*
I wonder if 'I' needs to be capitalized, as it starts a sentence:
* Authenticator for a particular realm is single threaded.
- * ie. if multiple threads need to get credentials from the user
+ * i.e. if multiple threads need to get credentials from the user
* at the same time, then all but the first will block until
No need for a separate review, if you agree to change i -> I.
With kind regards,
Ivan
On 11/12/18 6:45 PM, Pavel Rappo wrote:
On 13 Nov 2018, at 00:35, Ivan Gerasimov wrote:
Do you want to change ie. -> i.e. here as well:
src/java.base/windows/native/libnet/net_util_md.c
- * 2. If the reqeusted port is 0 (ie. any port) then we try to bind in v4 space
+ * 2. If the requested port is 0 (ie. any port) then we try to bind in v4 space
Thanks. I have addressed "eg." too, please see the result here:
http://cr.openjdk.java.net/~prappo/8213490/webrev.02/
And a couple more of duplicate words to remove:
jdk/internal/net/http/Http1AsyncReceiver.java:// If the queue is not
empty, wait until it it is empty before
jdk/internal/net/http/Http2Connection.java:// if true, the the stream may
be assigned to this connection
After I have noticed this this pattern [*] I wrote a tiny tool to help me search
for occurrences of it in comments and javadocs. It's just a little bit more
complicated than a bunch of regexes, as javadoc structures (e.g. "{@link method
method}" or "@param number number", etc.) would otherwise yield too many false
positives. Your comment reminded me that I forgot about single-line comments.
The tool has been updated. Thanks.
[*] Pun intended
--
With kind regards,
Ivan Gerasimov
Re: RFR [12] 8213490: Networking area nano cleanup
Hi Pavel! All looks good to me! Do you want to change ie. -> i.e. here as well: src/java.base/windows/native/libnet/net_util_md.c - * 2. If the reqeusted port is 0 (*ie*. any port) then we try to bind in v4 space + * 2. If the requested port is 0 (*ie*. any port) then we try to bind in v4 space And a couple more of duplicate words to remove: jdk/internal/net/http/Http1AsyncReceiver.java:// If the queue is not empty, wait until *it it* is empty before jdk/internal/net/http/Http2Connection.java:// if true, *the the* stream may be assigned to this connection With kind regards, Ivan On 11/12/18 3:51 PM, Pavel Rappo wrote: Daniel, Alan, I excluded the update from the draft to the RFC and created a separate bug for it: [P5] 8213757: Investigate the possibility of updating the reference to the spec in java.net.Inet6Address I added the changes to the URI class from JDK-8213490, which then effectively became a duplicate. Please have a look at the result here: http://cr.openjdk.java.net/~prappo/8213490/webrev.01/ -Pavel On 12 Nov 2018, at 18:14, Alan Bateman wrote: On 12/11/2018 17:30, Daniel Fuchs wrote: Hi Pavel, The typos fixes look OK to me - I'll let Michael/Chris? who have more knowledge on the history of the Inet6Address impl to validate the new link - though I suspect that's OK. It will need a CSR because it changes Inet6Address to specify that it can be extended with scoped addresses described by RFC 4007. It might need analysis to understand the differences between the draft and RFC 4007 (just in case it brings up implementation or conformance test issues). -Alan -- With kind regards, Ivan Gerasimov
Re: [PATCH] SOCK_CLOEXEC for opening sockets
Hi Chris!
A couple of minor comments.
1)
if (s != -1 || (s == -1 && errno != EINVAL)) {
This can be simplified to
if (s != -1 || errno != EINVAL) {
2)
What about sockets created with accept(): Shouldn't NET_Accept be
modified to set O_CLOEXEC as well?
On Linux accept4() can be used to pass SOCK_CLOEXEC flag.
With kind regards,
Ivan
On 7/25/18 5:49 AM, Chris Hegarty wrote:
Alan,
On 25 Jul 2018, at 08:24, Alan Bateman wrote:
...
As I said previously, the patch isn't complete so native code calling fork/exec
may still have to deal with other file descriptors that are inherited into the
child. I don't object to doing this in phases of course but somehow we have
managed to get by for 20 years without this being an issue.
I added the following to the JIRA description to make this clear:
"This JIRA issue, by itself, does not completely resolve the problem
of native code calling fork/exec, it may still have to deal with other
file descriptors that are inherited by the child. Instead this JIRA
issue is targeted at the socket and channels areas only, other areas
should be tackled on a phased approach though separate JIRA
issues."
The updates to the various site to use the NET_* functions are fine. However, I
think the new functions in net_util_md.c could be cleaner. I think it would be
better to fallback to socket/socketpair + fcntl when the initial call fails
with EINVAL.
Agreed. How about this ( trying to reduce the ifdef blocks, and
keep them relatively clean ) :
---
JNIEXPORT int JNICALL
NET_Socket(int domain, int type, int protocol) {
int s;
#if defined(SOCK_CLOEXEC)
s = socket(domain, type | SOCK_CLOEXEC, protocol);
if (s != -1 || (s == -1 && errno != EINVAL)) {
return s;
}
#endif
s = socket(domain, type, protocol);
if (s != -1) {
// Best effort - return value is intentionally ignored since the socket
// was successfully created.
fcntl(s, F_SETFD, FD_CLOEXEC);
}
return s;
}
---
Updated webrev:
http://cr.openjdk.java.net/~chegar/8207335/webrev.01/
-Chris.
--
With kind regards,
Ivan Gerasimov
Re: RFR : 8205959 : Do not restart close if errno is EINTR
Thanks David! On 6/27/18 4:23 PM, David Lloyd wrote: According to http://man7.org/linux/man-pages/man2/close.2.html it is currently platform-dependent whether close() must or must not (seems to be no middle ground) be retried. Might have to do some #ifdef guarding? Right. Here I'm patching only the Linux-specific file src/java.base/linux/native/libnet/linux_close.c So no #ifdefs seem necessary. MacOS variant also uses the same pattern, but I'm not touching it exactly because the behavior not quite clear on that platform. On Solaris we already call close(fd) with no retrying. With kind regards, Ivan -- - DML On Jun 27, 2018, at 6:15 PM, Ivan Gerasimov <mailto:ivan.gerasi...@oracle.com>> wrote: Hello! When closing a socket via NET_SocketClose(int fd), a close(fd) is called. The later is wrapped in a retry-loop, which is wrong because close() is not restartable. The `man 2 close` states: """ ... close() should not be retried after an EINTR since this may cause a reused descriptor from another thread to be closed. """ Would you please help review a trivial fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8205959 WEBREV: http://cr.openjdk.java.net/~igerasim/8205959/00/webrev/ <http://cr.openjdk.java.net/%7Eigerasim/8205959/00/webrev/> Thanks in advance! -- With kind regards, Ivan Gerasimov -- With kind regards, Ivan Gerasimov
RFR : 8205959 : Do not restart close if errno is EINTR
Hello! When closing a socket via NET_SocketClose(int fd), a close(fd) is called. The later is wrapped in a retry-loop, which is wrong because close() is not restartable. The `man 2 close` states: """ ... close() should not be retried after an EINTR since this may cause a reused descriptor from another thread to be closed. """ Would you please help review a trivial fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8205959 WEBREV: http://cr.openjdk.java.net/~igerasim/8205959/00/webrev/ Thanks in advance! -- With kind regards, Ivan Gerasimov
Re: RFR: 8205342: windows : potential memleaks in getAdapter(s) in NetworkInterface_winXP.c
Thanks Matthias! The last webrev looks good to me! With kind regards, Ivan On 6/25/18 7:20 AM, Baesken, Matthias wrote: Hi Ivan , I removed the memset calls as suggested by Thomas , makes the change even a little bit shorter ; and replaced the fix “100” by sizeof in the print calls . New webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8205342.2/ <http://cr.openjdk.java.net/%7Embaesken/webrevs/8205342.2/> Best regards, Matthias *From:*Ivan Gerasimov [mailto:ivan.gerasi...@oracle.com] *Sent:* Samstag, 23. Juni 2018 01:52 *To:* Baesken, Matthias ; net-dev@openjdk.java.net *Cc:* Alan Bateman ; Stuefe, Thomas *Subject:* Re: RFR: 8205342: windows : potential memleaks in getAdapter(s) in NetworkInterface_winXP.c Hello Matthias! Thanks for the fix! On 6/22/18 6:08 AM, Baesken, Matthias wrote: Hello Alan, Thomas , I adjusted the line lengths and created a new webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8205342.1/ <http://cr.openjdk.java.net/%7Embaesken/webrevs/8205342.1/> I considered replacing the 100 for error_msg_buf size by a define (or maybe const int?) , should I do so ? I'd prefer to have hardcoded 100 replaced with sizeof(error_msg_buf) at lines 125 and 195. And with sizeof(error_msg_buf) / sizeof(error_msg_buf[0]) at lines 126 and 196. I understand that it is highly unlikely that type of error_msg_buf will ever change, but I think it would express the intention for the argument values clearer. With kind regards, Ivan Best regards, Matthias *From:*Alan Bateman [mailto:alan.bate...@oracle.com] *Sent:* Mittwoch, 20. Juni 2018 10:45 *To:* Baesken, Matthias <mailto:matthias.baes...@sap.com>; net-dev@openjdk.java.net <mailto:net-dev@openjdk.java.net> *Subject:* Re: RFR: 8205342: windows : potential memleaks in getAdapter(s) in NetworkInterface_winXP.c On 20/06/2018 09:07, Baesken, Matthias wrote: Hello . Please review this small fix that fixes potential memory leaks in getAdapter(s) in NetworkInterface_winXP.c and simplifies the coding a bit too . Currently when generating error messages , some memory is malloc-ed for the error messages , but not always freed . Bug: https://bugs.openjdk.java.net/browse/JDK-8205342 webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8205342/ <http://cr.openjdk.java.net/%7Embaesken/webrevs/8205342/> Can you fix the line lengths to make it consistent with original code? That will make it easier to look at side-by-side diffs. -Alan -- With kind regards, Ivan Gerasimov -- With kind regards, Ivan Gerasimov
Re: RFR: 8205342: windows : potential memleaks in getAdapter(s) in NetworkInterface_winXP.c
Hello Matthias! Thanks for the fix! On 6/22/18 6:08 AM, Baesken, Matthias wrote: Hello Alan, Thomas , I adjusted the line lengths and created a new webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8205342.1/ <http://cr.openjdk.java.net/%7Embaesken/webrevs/8205342.1/> I considered replacing the 100 for error_msg_buf size by a define (or maybe const int?) , should I do so ? I'd prefer to have hardcoded 100 replaced with sizeof(error_msg_buf) at lines 125 and 195. And with sizeof(error_msg_buf) / sizeof(error_msg_buf[0]) at lines 126 and 196. I understand that it is highly unlikely that type of error_msg_buf will ever change, but I think it would express the intention for the argument values clearer. With kind regards, Ivan Best regards, Matthias *From:*Alan Bateman [mailto:alan.bate...@oracle.com] *Sent:* Mittwoch, 20. Juni 2018 10:45 *To:* Baesken, Matthias ; net-dev@openjdk.java.net *Subject:* Re: RFR: 8205342: windows : potential memleaks in getAdapter(s) in NetworkInterface_winXP.c On 20/06/2018 09:07, Baesken, Matthias wrote: Hello . Please review this small fix that fixes potential memory leaks in getAdapter(s) in NetworkInterface_winXP.c and simplifies the coding a bit too . Currently when generating error messages , some memory is malloc-ed for the error messages , but not always freed . Bug: https://bugs.openjdk.java.net/browse/JDK-8205342 webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8205342/ <http://cr.openjdk.java.net/%7Embaesken/webrevs/8205342/> Can you fix the line lengths to make it consistent with original code? That will make it easier to look at side-by-side diffs. -Alan -- With kind regards, Ivan Gerasimov
Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes
On 5/24/18 10:25 PM, Weijun Wang wrote: https://stackoverflow.com/questions/27509061/macro-to-avoid-duplicate-case-value Someone has already used them in a switch expression... Interesting! I like the very first solution: #if EAGAIN != EWOULDBLOCK case EAGAIN: #endif case EWOULDBLOCK: It is descriptive enough and compiles optimally. Fortunately, we don't have to care about it now, as the fix only adds checks of errno right after calls to system functions and does not store the errno value anywhere to analyze it later. -- With kind regards, Ivan Gerasimov
Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes
Hi David!
If gcc, then we already have the same test for both constants in code
with no issues.
For example, java.base/unix/native/libnet/SocketInputStream.c, in
NET_ReadWithTimeout():
result = NET_NonBlockingRead(fd, bufP, len);
if (result == -1 && ((errno == EAGAIN) || (errno ==
EWOULDBLOCK))) {
If javac, then, I was thinking about it too, but I don't have a good
a universal solution to propose right now.
javac should treat these symbolically rather than based on actual
value, so I don't see any issue there. It's the C compiler that sees
the raw value after preprocessing and so sees "duplicate" clauses.
If fact, as UnixContant.java is built from the template file
UnixConstants.java.template, which is processed with cpp preprocessor,
javac also sees the raw values.
So if someone decides to write something like
switch (exc.errno()) {
case UnixConstants.EAGAIN:
case UnixConstants.EWOULDBLOCK:
}
then there will be compile time trouble on some platforms and no issues
on the others.
Fortunately, UnixContant class is package private, so it is quite
unlikely to happen.
I was even thinking about proposing a language-level enhancement: If a
switch statement contains duplicate cases *and* these are combined, then
treat them as just one case.
Though the use case it too limited to justify the need :)
With kind regards,
Ivan
Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes
Hi Wiijun!
On 5/24/18 10:13 PM, Weijun Wang wrote:
On May 25, 2018, at 11:58 AM, Ivan Gerasimov <ivan.gerasi...@oracle.com> wrote:
I also wonder whether a smart compiler might not flag code where the errors do
infact have the same value:
if (errno == 11 || errno == 11) ...
At least gcc -O completely removes the second redundant test, so no observable
changes is expected on supported platforms.
And it silently compiles without showing any warning, right? Good if yes.
--Max
Yep, all is good.
I've built/tested the patched JDK on all supported platforms with no issues.
And we already have places, where both EAGAIN and EWOULDBLOCK are used
in one if clause (as I just replied to David):
java.base/unix/native/libnet/SocketInputStream.c, in NET_ReadWithTimeout():
result = NET_NonBlockingRead(fd, bufP, len);
if (result == -1 && ((errno == EAGAIN) || (errno ==
EWOULDBLOCK))) {
So the fix basically proposes to use this approach consistently.
With kind regards,
Ivan
--
With kind regards,
Ivan Gerasimov
Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes
Hi David!
On 5/24/18 9:45 PM, David Holmes wrote:
I looked but did not review - it was just an observation :)
Well, thank you anyway :)
It seems pointless to double up these condition checks everywhere
just in case there is some platform (do we know of one?) where this
may be necessary.
That's exactly what man pages suggest: "...a portable application
should check for both..."
Yes but that's the native code that calls the library methods. That
doesn't necessarily mean we have to propagate the ambiguity through
our own native wrappers and/or Java code.
Ah, I didn't immediately understood you're talking about constants in
UnixConstants.java and LinuxWatchService.java.
This part might probably be skipped (because we know that on Linux the
constants have the same values), but I thought it's better it add it for
consistency.
In other parts of the fix we do treat the constants uniformly and
propagate some non-ambiguous value to Java, like returning
IOS_UNAVAILABLE in most cases.
And yes, there exist such platforms.
I also wonder whether a smart compiler might not flag code where the
errors do infact have the same value:
if (errno == 11 || errno == 11) ...
At least gcc -O completely removes the second redundant test, so no
observable changes is expected on supported platforms.
I'm more worried about a new compiler warning - especially if you
happened to use them in a switch! - resulting in future build failures.
What compiler do you mean: gcc or javac?
If gcc, then we already have the same test for both constants in code
with no issues.
For example, java.base/unix/native/libnet/SocketInputStream.c, in
NET_ReadWithTimeout():
result = NET_NonBlockingRead(fd, bufP, len);
if (result == -1 && ((errno == EAGAIN) || (errno ==
EWOULDBLOCK))) {
If javac, then, I was thinking about it too, but I don't have a good a
universal solution to propose right now.
If one day someone needs to use these (platform dependent by definition)
constants in switch, one will need to invent something to workaround the
fact that some constants may have the same values on some platforms.
With respect to EAGAIN and EWOULDBLOCK, it will be caught early enough
because it will fail during the very first build on any currently
supported Unix platform.
With kind regards,
Ivan
Cheers,
David
With kind regards,
Ivan
Cheers,
David
On 25/05/2018 6:57 AM, Ivan Gerasimov wrote:
Hello!
On Unix systems several system calls (including pread, read, readv,
recvfrom, recvmsg, send, sendfile, sendmsg, sendto) may set errno
to either EAGAIN or EWOULDBLOCK on the same condition.
On Linux these two constants are the same, but they are not
required to be the same.
For example, here's an extract from the Linux man page of send():
EAGAIN or EWOULDBLOCK
The socket is marked nonblocking and the requested operation
would block. POSIX.1-2001 allows either error to be returned for
this case, and does not require these constants to have the same
value, so a portable application should check for both possibilities.
We should check for both error codes when appropriate.
Would you please help review the fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8203369
WEBREV: http://cr.openjdk.java.net/~igerasim/8203369/00/webrev/
Thanks!
--
With kind regards,
Ivan Gerasimov
Re: RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes
Hi David! Thank you for reviewing the fix! On 5/24/18 7:44 PM, David Holmes wrote: Hi Ivan, Just a thought, but just because the actual native function may return either code, that doesn't mean our native wrapper can't treat them the same and present them to the Java code as one error? Currently in some places we check for only one of the values (on the supported platforms we could have being checking for the other with exactly same effect). In other places we already check for both values, so it is proposed to do it consistently with accordance to the documentation. It seems pointless to double up these condition checks everywhere just in case there is some platform (do we know of one?) where this may be necessary. That's exactly what man pages suggest: "...a portable application should check for both..." And yes, there exist such platforms. I also wonder whether a smart compiler might not flag code where the errors do infact have the same value: if (errno == 11 || errno == 11) ... At least gcc -O completely removes the second redundant test, so no observable changes is expected on supported platforms. With kind regards, Ivan Cheers, David On 25/05/2018 6:57 AM, Ivan Gerasimov wrote: Hello! On Unix systems several system calls (including pread, read, readv, recvfrom, recvmsg, send, sendfile, sendmsg, sendto) may set errno to either EAGAIN or EWOULDBLOCK on the same condition. On Linux these two constants are the same, but they are not required to be the same. For example, here's an extract from the Linux man page of send(): EAGAIN or EWOULDBLOCK The socket is marked nonblocking and the requested operation would block. POSIX.1-2001 allows either error to be returned for this case, and does not require these constants to have the same value, so a portable application should check for both possibilities. We should check for both error codes when appropriate. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8203369 WEBREV: http://cr.openjdk.java.net/~igerasim/8203369/00/webrev/ Thanks! -- With kind regards, Ivan Gerasimov
RFR 8203369 : Check for both EAGAIN and EWOULDBLOCK error codes
Hello! On Unix systems several system calls (including pread, read, readv, recvfrom, recvmsg, send, sendfile, sendmsg, sendto) may set errno to either EAGAIN or EWOULDBLOCK on the same condition. On Linux these two constants are the same, but they are not required to be the same. For example, here's an extract from the Linux man page of send(): EAGAIN or EWOULDBLOCK The socket is marked nonblocking and the requested operation would block. POSIX.1-2001 allows either error to be returned for this case, and does not require these constants to have the same value, so a portable application should check for both possibilities. We should check for both error codes when appropriate. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8203369 WEBREV: http://cr.openjdk.java.net/~igerasim/8203369/00/webrev/ Thanks! -- With kind regards, Ivan Gerasimov
Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive
Thanks Vyom!
I like it much better now.
The last minorish comment:
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Set<SocketOption> options = new HashSet<>(5);
Please note that the constructor of HashSet wants the initial capacity
as the argument, not the expected number of elements.
So in this case it would be more accurate to have (7), so that 7 * 0.75
= 5.25 > 5.
Practically, it wouldn't make any difference, as both 5 and 7 would be
rounded up to 8 anyways.
However, I would recommend using the default constructor just to avoid
confusion.
Or, alternatively, collect the options to an ArrayList of desired
capacity and then make unmodifiable set with Set.copyOf(list).
No further comments from my side :)
With kind regards,
Ivan
On 5/12/18 2:21 AM, vyom tewari wrote:
Hi Ivan,
Thanks for review, please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.4/index.html).
Please find my answers in-line.
Thanks,
vyom
On Saturday 12 May 2018 11:15 AM, Ivan Gerasimov wrote:
Hi Vyom!
1)
src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java
Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then
Collectors.toUnmodifiableSet could be used for convenience:
return options.stream()
.filter((option) -> !option.name().startsWith("TCP_"))
.collect(Collectors.toUnmodifiableSet());
Also, I think Alan suggested to filter out UDP_* options for
SOCK_STREAM here.
fixed
2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java
If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other
files, then the line 80 wouldn't become too long.
The same applies to
src/java.base/unix/classes/java/net/PlainSocketImpl.java
fixed
3)
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Was it intentional that when flowSupported == true, quickAckSupported
== false, keepAliveOptSupported == true, the TCP_KEEP* options are
*not* added to the resulting set?
If not, then can this set population be organized in more linear
way: Just create an ArrayList, conditionally fill it in and return
unmodifiable set with Set.of(list.toArray()).
of course not, i don't know but i always prefer complex nested
"if-else" then linear ifs :) fixed as well.
Nit: please place the equal sign at line 172 consistently with the
other two inits above.
fixed.
With kind regards,
Ivan
On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html).
I address most of the review comments.
Vyom
On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:
On 11 May 2018, at 01:04, Alan Bateman <alan.bate...@oracle.com>
wrote:
On 10/05/2018 16:21, vyom tewari wrote:
Hi,
Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html)
...
It would be better if the channel implementation didn't static
import ExtendedSocketOptions.getInstance as that is a very generic
method method name. As I mentioned previously, you could simplify
all these usages if you add the following to
sun.net.ext.ExtendedSocketOption
static Set<SocketOption> options(int type) { return
getInstance().options(type)); }
+1
A minor comment on tests is that they can use List.of instead of
Arrays.asList.
+1
Otherwise, this looks very good.
-Chris.
P.S. A separate issue, but when reviewing this it reminded me that
we should deprecate-for-removal jdk/net/Sockets.java. It’s
functionality is already supported by a standard API.
--
With kind regards,
Ivan Gerasimov
Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive
Hi Vyom!
1)
src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java
Thank you for fixing ExtendingSocketOption.options0().
It may be better to make the returned set unmodifiable, and then
Collectors.toUnmodifiableSet could be used for convenience:
return options.stream()
.filter((option) -> !option.name().startsWith("TCP_"))
.collect(Collectors.toUnmodifiableSet());
Also, I think Alan suggested to filter out UDP_* options for SOCK_STREAM
here.
2)
src/java.base/unix/classes/java/net/PlainDatagramSocketImpl.java
If you static-import ExtendedSocketOptions.SOCK_DGRAM as in other files,
then the line 80 wouldn't become too long.
The same applies to src/java.base/unix/classes/java/net/PlainSocketImpl.java
3)
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Was it intentional that when flowSupported == true, quickAckSupported ==
false, keepAliveOptSupported == true, the TCP_KEEP* options are *not*
added to the resulting set?
If not, then can this set population be organized in more linear way:
Just create an ArrayList, conditionally fill it in and return
unmodifiable set with Set.of(list.toArray()).
Nit: please place the equal sign at line 172 consistently with the
other two inits above.
With kind regards,
Ivan
On 5/11/18 7:43 PM, vyom tewari wrote:
thanks all for review, please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.3/index.html).
I address most of the review comments.
Vyom
On Saturday 12 May 2018 12:01 AM, Chris Hegarty wrote:
On 11 May 2018, at 01:04, Alan Bateman <alan.bate...@oracle.com> wrote:
On 10/05/2018 16:21, vyom tewari wrote:
Hi,
Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html)
...
It would be better if the channel implementation didn't static
import ExtendedSocketOptions.getInstance as that is a very generic
method method name. As I mentioned previously, you could simplify
all these usages if you add the following to
sun.net.ext.ExtendedSocketOption
static Set<SocketOption> options(int type) { return
getInstance().options(type)); }
+1
A minor comment on tests is that they can use List.of instead of
Arrays.asList.
+1
Otherwise, this looks very good.
-Chris.
P.S. A separate issue, but when reviewing this it reminded me that we
should deprecate-for-removal jdk/net/Sockets.java. It’s functionality
is already supported by a standard API.
--
With kind regards,
Ivan Gerasimov
Re: RFR:8194298 Add support for per Socket configuration of TCP keepalive
Hi Vyom!
A few random comments:
1)
src/java.base/share/classes/sun/net/ext/ExtendedSocketOptions.java
Collections.emptySet() produces an immutable set, so it must not be
possible to add elements to it.
Is there a test that verifies that ExtendedSocketOption.options(short)
works as expected?
2)
In several files:
Personally, I found it counter-intuitive that static
ExtendedSocketOptions.getInstance() is imported and used without the
class name.
It is hard to realize from the context to which class the getInstance()
call belongs to.
3)
src/jdk.net/linux/native/libextnet/LinuxSocketOptions.c
Formatting nits: Please add a space after comma at line 125, add an
empty line after 128.
4)
src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java
Please fix formatting at lines 355-358 (spaces, else after })
Can the new set/get methods at 305-325 be declared private?
5)
src/jdk.net/share/classes/jdk/net/Sockets.java
It is probably a matter of preference, but it might be slightly more
efficient to call set.add() three times instead of calling
set.addAll(Set.of(...)).
Similarly, it may be a little bit faster to do (set.contains(A) &&
set.contains(B) && set.contains(C)) instead of set.containsAll(Set.of(A,
B, C)).
6)
test/jdk/java/nio/channels/AsynchronousServerSocketChannel/Basic.java
indentation is broken at lines 176-183
7)
test/jdk/java/nio/channels/AsynchronousSocketChannel/Basic.java
Extra space in the indentation at 192 :)
8)
test/jdk/java/nio/channels/SocketChannel/SocketOptionTests.java
Would it make sense to add a test that either all three options
{TCP_KEEPCOUNT, TCP_KEEPIDLE, TCP_KEEPINTERVAL} are supported or none of
them?
With kind regards,
Ivan
On 5/10/18 8:21 AM, vyom tewari wrote:
Hi,
Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.2/index.html).
I incorporated most of the review comments. Chris as you suggested in
below mail i did not added the note for upper-bound because values are
also OS specific.
Thanks,
Vyom
On Monday 23 April 2018 07:26 PM, Chris Hegarty wrote:
On 23/04/18 13:34, Alan Bateman wrote:
On 23/04/2018 13:05, vyom tewari wrote:
Hi,
Please find the latest
webrev(http://cr.openjdk.java.net/~vtewari/8194298/webrev0.1/index.html).
I incorporated most of the review comments.
This looks much better but I think the second paragraph of the spec
of each option needs to be inverted so that it states clearly what
the options does before it gets into the background of SO_KEEPALIVE.
For example, TCP_KEEPALIVE should say that it sets the idle period
for keep alive before it explains SO_KEEPALIVE.
Mea culpa, I ordered the paragraphs as they are to be consistent
with TCP_QUICKACK. I don't have any objection if they are reversed.
The currently wording also begs questions as to whether the socket
option means anything when SO_KEEPALIVE is not enabled.
It's implicit. The option can still be set and its value retrieved.
Also it probably should say something about whether it can be
changed at any time or not.
You can set it any time. Of course, it may not be effective
immediately, depending on the exact state of the socket.
We may also want to add a note that the accepted values may
have an upper-bound. For example, the following is the largest
set of values that I can set on my Ubuntu Linux, without an
exception being thrown [*].
TCP_KEEPIDLE = 32767
TCP_KEEPINTERVAL = 32767
TCP_KEEPCOUNT = 127
-Chris.
[*] "java.net.SocketException: Invalid argument" when a given
value is "too" large.
--
With kind regards,
Ivan Gerasimov
Re: RFR 8202558 : Access to freed memory in java.base/unix/native/libnet/net_util_md.c
I just realized that we've got a similar issue in the same file in the function initLocalIfs(). The webrev was updated in place to fix this issue as well. With kind regards, Ivan On 5/2/18 12:54 PM, Ivan Gerasimov wrote: Hello! The function needsLoopbackRoute() calls initLoopbackRoutes() to initialize two global variables: loRoutes and nRoutes. If realloc() fails at line 582 then loRoutes is freed, but nRoutes is left positive. Then, in needsLoopbackRoute() the already freed memory will be accessed. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202558 WEBREV: http://cr.openjdk.java.net/~igerasim/8202558/00/webrev/ Thanks!
RFR 8202558 : Access to freed memory in java.base/unix/native/libnet/net_util_md.c
Hello! The function needsLoopbackRoute() calls initLoopbackRoutes() to initialize two global variables: loRoutes and nRoutes. If realloc() fails at line 582 then loRoutes is freed, but nRoutes is left positive. Then, in needsLoopbackRoute() the already freed memory will be accessed. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202558 WEBREV: http://cr.openjdk.java.net/~igerasim/8202558/00/webrev/ Thanks! -- With kind regards, Ivan Gerasimov
Re: RFR 8202154 : Remove unused code in java.base/windows/native/libnet
Hello again! A few other files contain obsolete code, so they can be combined together in one fix. The webrev was updated in place: http://cr.openjdk.java.net/~igerasim/8202154/00/webrev/ Here's the summary of additional changes: - sun.net.PortConfig.getLower()/getUpper() always return the same range, so it can be defined with constants, - NET_GetDefaultTOS() always returns zero, so it can be removed. Would you please help review this? With kind regards, Ivan On 4/23/18 2:29 PM, Ivan Gerasimov wrote: Hello! Some code in TwoStacksPlainDatagramSocketImpl.c is only relevant for earlier versions of Windows, which are no longer supported as of JDK 11. This code can be safely removed. Also removing an unused argument at DualStackPlainDatagramSocketImpl.socketCreate(). Would you please help review this cleanup? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202154 WEBREV: http://cr.openjdk.java.net/~igerasim/8202154/00/webrev/ Thanks in advance! -- With kind regards, Ivan Gerasimov
RFR 8202154 : Remove unused code in TwoStacksPlainDatagramSocketImpl.c
Hello! Some code in TwoStacksPlainDatagramSocketImpl.c is only relevant for earlier versions of Windows, which are no longer supported as of JDK 11. This code can be safely removed. Also removing an unused argument at DualStackPlainDatagramSocketImpl.socketCreate(). Would you please help review this cleanup? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8202154 WEBREV: http://cr.openjdk.java.net/~igerasim/8202154/00/webrev/ Thanks in advance! -- With kind regards, Ivan Gerasimov
RFR 8202091 : Rename DualStackPlainSocketImpl to PlainSocketImpl [win]
Hello! After integrating the fix for JDK-8201510, there is only one implementation of PlainSocketImpl left on Windows. It is proposed to just rename DualStackPlainSocketImpl to PlainSocketImpl and avoid the indirection. Some minor cleanup was done with this change: - dropping static PlainSocketImpl.isReusePortAvailable() -- it wasn't called anyway and just hid the same named function in the super class, - minor re-formatting. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8194534 WEBREV: http://javaweb.us.oracle.com/~igerasim/webrevs/8194534/00/webrev/ Thanks in advance! -- With kind regards, Ivan Gerasimov
Re: RFR 8201510 : Merge TwoStacksPlainSocketImpl into DualStackPlainSocketImpl [win]
Thanks again Chris! On 4/19/18 1:50 AM, Chris Hegarty wrote: On 19 Apr 2018, at 09:44, Ivan Gerasimov <ivan.gerasi...@oracle.com <mailto:ivan.gerasi...@oracle.com>> wrote: ... WEBREV:http://cr.openjdk.java.net/~igerasim/8201510/01/webrev/ <http://cr.openjdk.java.net/%7Eigerasim/8201510/01/webrev/> Thanks Ivan. This looks good, and thanks for updating the existing tests to cover the property values ( you know that jtreg now supports spanning @run tags over multiple lines, e.g. [*] ;-) ) Okay, good to know! I'll break up the long @run lines before pushing. With kind regards, Ivan -Chris. [*] http://hg.openjdk.java.net/jdk/jdk/file/tip/test/jdk/java/net/httpclient/BasicRedirectTest.java#l36 -- With kind regards, Ivan Gerasimov
Re: RFR 8201510 : Merge TwoStacksPlainSocketImpl into DualStackPlainSocketImpl [win]
Thanks you Chris for reviewing this!
On 4/18/18 9:06 AM, Chris Hegarty wrote:
Ivan,
On 16/04/18 17:29, Ivan Gerasimov wrote:
...
WEBREV: http://cr.openjdk.java.net/~igerasim/8201510/00/webrev/
I think this is mostly good. Just one comment.
I'm not sure that this is correct.
--- OLD ---
60 String exclBindProp = AccessController.doPrivileged(
61 new GetPropertyAction("sun.net.useExclusiveBind",
""));
62 exclusiveBind = (exclBindProp.isEmpty())
63 ? true
64 : Boolean.parseBoolean(exclBindProp);
--- NEW ---
private static final boolean useExclusiveBind =
55 Boolean.parseBoolean(AccessController.doPrivileged(
56 new GetPropertyAction("sun.net.useExclusiveBind",
"true")));
Exclusive bind should be true iif:
1) it is defined and has no value, or
2) if is defined and has a value of `true`.
Oh. Good catch! Thanks!
I restored the logic here in the updated webrev.
I thought we had tests for this, but maybe not if you are not
seeing test failures.
I found only two tests that set useExclusiveBind. They seem to ignore
false positive results, that's why they didn't fail.
I updated them to test with useExclusiveBind set either to 'true' or to
the empty value.
Here's the updated webrev:
WEBREV: http://cr.openjdk.java.net/~igerasim/8201510/01/webrev/
With kind regards,
Ivan
-Chris.
--
With kind regards,
Ivan Gerasimov
RFR 8201510 : Merge TwoStacksPlainSocketImpl into DualStackPlainSocketImpl [win]
Hello! After integrating the fix for JDK-8198358 there are only a few differences left in these two implementations, so it is relatively easy to merge them together. Some cleanup was done along the way: - unused argument in socket0() was removed, - tests of the IP family were moved from native code to Java layer, - SetHandleInformation((HANDLE)(UINT_PTR)fd, HANDLE_FLAG_INHERIT, FALSE) was removed from socket0 (as it is already done inside NET_Socket()), - if (WSAGetLastError() == -2) branch removed from accept0(), as it can never succeed anyway (in TwoStacks it was testing for newfd == -2, which doesn't seem correct either), - in accept0() added a check if (*env)->NewObject() was unsuccessful. Also a regression test was added to verify that bind() and connect() reject Inet6Address when the system option java.net.preferIPv4Stack is set to true. The patched JDK was successfully built and all the tests, including the new one, passed well. The new test also passed well on all other platforms. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8201510 WEBREV: http://cr.openjdk.java.net/~igerasim/8201510/00/webrev/ Thanks in advance! -- With kind regards, Ivan Gerasimov
Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]
Thank you Christoph and Chris for the review!
I made the following suggested changes:
- fdAccess made private static final in both TwoStacks and DualStack.
- removed redundant check IS_NULL(iaObj) at bind0 (indeed, the address
was already checked at Java level).
For now, I would prefer to keep the checks for family == IPv4 as they are.
I totally agree they should be moved to Java, but I'm planning to do
this during the TwoStacks-DualStack merge step.
I'm also planning to add a regtest to make sure that Inet6Address is
rejected when fed to a IPv4-only socked.
Are we good to push now?
Please let me know, if you'd like me to send the updated webrev set.
With kind regards,
Ivan
On 3/27/18 6:47 AM, Chris Hegarty wrote:
Ivan,
On 26 Mar 2018, at 23:08, Ivan Gerasimov <ivan.gerasi...@oracle.com> wrote:
Hello everyone!
A few modifications were done to the patch.
Below is the list of updated parts of the patch with comments about what has
changed, comparing to the previous version of the patch.
The patched JDK builds fine and all the tests pass well.
WEBREV-000: http://cr.openjdk.java.net/~igerasim/8198358/04/000/webrev/
Ok.
WEBREV-001: http://cr.openjdk.java.net/~igerasim/8198358/04/001/webrev/
Ok.
WEBREV-002: http://cr.openjdk.java.net/~igerasim/8198358/04/002/webrev/
Can we make fdAccess final ( and in DualStack too )
static *final* JavaIOFileDescriptorAccess fdAccess
JNIEXPORT void JNICALL Java_java_net_TwoStacksPlainSocketImpl_bind0 ...
142 if (IS_NULL(iaObj)) {
143 JNU_ThrowNullPointerException(env, "inet address argument");
144 return;
145 }
146
147 family = getInetAddress_family(env, iaObj);
148 if (family != java_net_InetAddress_IPv4) {
149 JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
150 "Protocol family not supported");
151 return;
152 }
The address is already null checked at the java level, so can be removed here,
no?
Can the family check be hoisted up into Java? I think this will help when it
comes
to the eventual merge with DualStack.
The native code will then align with DualStack’s. I prefer to reduce and align
the native layers as much as possible. It’s easier to deal with differences at
the Java level.
WEBREV-003: http://cr.openjdk.java.net/~igerasim/8198358/04/003/webrev/
- Rebased to reflect recent changes to socketListen()
Ok.
WEBREV-004: http://cr.openjdk.java.net/~igerasim/8198358/04/004/webrev/
Ok.
WEBREV-005: http://cr.openjdk.java.net/~igerasim/8198358/04/005/webrev/
Ok.
WEBREV-006: http://cr.openjdk.java.net/~igerasim/8198358/04/006/webrev/
Ok.
WEBREV-007: http://cr.openjdk.java.net/~igerasim/8198358/04/007/webrev/
Ok.
WEBREV-008: http://cr.openjdk.java.net/~igerasim/8198358/04/008/webrev/
- The check the line 400 was changed to test if (newfd < 0) and then if (newfd
== -2).
I don't think the later is quite accurate (cannot find in the documentation
that accept can return -2), but it is how it was done in TwoStacks originally,
so no regression is expected.
Ok.
413 if (sa.sa.sa_family != AF_INET) {
414 JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
415 "Protocol family not supported");
416 NET_SocketClose(newfd);
417 return -1;
418 }
This check could be hoisted up to the Java-level too, no? Again, I think it
would be easier to deal with this kinda differences in Java.
WEBREV-009: http://cr.openjdk.java.net/~igerasim/8198358/04/009/webrev/
- Added missing 'return -1;' at the line 188.
Can the family be checked at the java level.
WEBREV-010: http://cr.openjdk.java.net/~igerasim/8198358/04/010/webrev/
Ok.
WEBREV-011: http://cr.openjdk.java.net/~igerasim/8198358/04/011/webrev/
Ok.
WEBREV-012: http://cr.openjdk.java.net/~igerasim/8198358/04/012/webrev/
Ok.
WEBREV-013: http://cr.openjdk.java.net/~igerasim/8198358/04/013/webrev/
Ok.
WEBREV-014: http://cr.openjdk.java.net/~igerasim/8198358/04/014/webrev/
Good.
-Chris.
--
With kind regards,
Ivan Gerasimov
Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]
On 3/25/18 12:11 PM, Alan Bateman wrote: On 25/03/2018 19:13, Ivan Gerasimov wrote: : In the code above, newfd was obtained from a call to accept() a few lines before this check. So, the Java code has no way of being aware of this socket, and it will never be closed unless we do it right here, before returning -1. The SocketImpl's fd is set with this code: (*env)->SetIntField(env, socketFdObj, IO_fd_fdID, fd); If there is any possibility of returning without a pending exception it will be registered with the cleaner. Okay. In the patched TwoStacksPlainSocketImp.c there is no setting FileDescriptor.fd from JNI left, so that the code will be safer in this aspect. At this point, I think we have to treat all code calling NET_SocketClose as a suspect until the regression is tracked down. -Alan -- With kind regards, Ivan Gerasimov
Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]
Hello everyone! A few modifications were done to the patch. Below is the list of updated parts of the patch with comments about what has changed, comparing to the previous version of the patch. The patched JDK builds fine and all the tests pass well. WEBREV-000: http://cr.openjdk.java.net/~igerasim/8198358/04/000/webrev/ WEBREV-001: http://cr.openjdk.java.net/~igerasim/8198358/04/001/webrev/ WEBREV-002: http://cr.openjdk.java.net/~igerasim/8198358/04/002/webrev/ WEBREV-003: http://cr.openjdk.java.net/~igerasim/8198358/04/003/webrev/ - Rebased to reflect recent changes to socketListen() WEBREV-004: http://cr.openjdk.java.net/~igerasim/8198358/04/004/webrev/ WEBREV-005: http://cr.openjdk.java.net/~igerasim/8198358/04/005/webrev/ WEBREV-006: http://cr.openjdk.java.net/~igerasim/8198358/04/006/webrev/ WEBREV-007: http://cr.openjdk.java.net/~igerasim/8198358/04/007/webrev/ WEBREV-008: http://cr.openjdk.java.net/~igerasim/8198358/04/008/webrev/ - The check the line 400 was changed to test if (newfd < 0) and then if (newfd == -2). I don't think the later is quite accurate (cannot find in the documentation that accept can return -2), but it is how it was done in TwoStacks originally, so no regression is expected. WEBREV-009: http://cr.openjdk.java.net/~igerasim/8198358/04/009/webrev/ - Added missing 'return -1;' at the line 188. WEBREV-010: http://cr.openjdk.java.net/~igerasim/8198358/04/010/webrev/ WEBREV-011: http://cr.openjdk.java.net/~igerasim/8198358/04/011/webrev/ WEBREV-012: http://cr.openjdk.java.net/~igerasim/8198358/04/012/webrev/ WEBREV-013: http://cr.openjdk.java.net/~igerasim/8198358/04/013/webrev/ WEBREV-014: http://cr.openjdk.java.net/~igerasim/8198358/04/014/webrev/ With kind regards, Ivan
Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]
Hi Chris!
On 3/25/18 6:37 AM, Chris Hegarty wrote:
On 23 Mar 2018, at 21:55, Chris Hegarty <chris.hega...@oracle.com
<mailto:chris.hega...@oracle.com>> wrote:
For reference, here's the combined webrev:
http://cr.openjdk.java.net/~igerasim/8198358/03/webrev/
<http://cr.openjdk.java.net/%7Eigerasim/8198358/03/webrev/>
I think this is good. And thanks for the additional test coverage,
this is great.
Actually, there is a couple of places in the new native code that
has the following pattern:
if (sa.sa.sa_family != AF_INET) {
JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
"Protocol family not supported");
NET_SocketClose(newfd);
return -1;
}
I would like to remove the NET_SocketClose, and allow the
exception propagating at the Java-level be responsible for
closing the socket, which will then do so in cooperation
with the socket cleaner ( it will effectively cancel the cleaner ).
-Chris.
In the code above, newfd was obtained from a call to accept() a few
lines before this check.
So, the Java code has no way of being aware of this socket, and it will
never be closed unless we do it right here, before returning -1.
I checked the other places, where family is checked to be AF_INET and
can confirm there's no other places where a socket gets closed.
With kind regards,
Ivan
--
With kind regards,
Ivan Gerasimov
Re: RFR 8200181 [11] Remove superflous non-IPv4 code from Java_java_net_TwoStacksPlainSocketImpl_socketListen
Hi Chris! On 3/23/18 11:48 AM, Chris Hegarty wrote: Ivan, On 23/03/18 18:38, Ivan Gerasimov wrote: Hi Chris! I was about to submit a patch for 8198358 that heavily modifies TwoStacksPlainSocketImpl to make it aligned with DualStackPlainSocketImpl. That patch will also remove this problematic code. I can rebase my patch, but wouldn't it be easier to proceed with that fix? There is an immediate urgency to have this possibly problematic code removed. We're seeing strangle rare intermittent failures that may be caused by it, and possibly the socket cleaner. It is extremely difficult to reproduce, so we want to eliminate this code from the equation. Okay, got it. I can rebase my patch after you push your fix. Regarding 8198358, I've been thinking again about this, and I think a better approach may be to just remove TwoStackPlainSocketImpl completely. The preferIPv4Stack can go through DualStackPlainSocketImpl with a indicator to create an AF_INET socket, just like on unix. I think this could be a more straight forward path forward, unless testing shows otherwise. Right. That's what I wanted to approach, but in a step-by-step way. Here's the list of other differences that remained after aligning the implementations: - in waitForConnect() I had to add throwing ConnectException if the last error was WSAEADDRNOTAVAIL (there was a regression test failure, when I tried to remove this special case), - in accept(), had to check the new file descriptor for == -2 (in the DualStacks the last error code is checked, which may be wrong), - had to add handling SO_TIMEOUT (in DualStack it's just ignored, but there was a test failure if I tried the same with TwoStacks). I agree that it looks too much work, given we're going to remove TwoStacks anyways, but I found it easier to do it this way to avoid the regressions. With kind regards, Ivan Maybe 8198358 could be amended to do this? Is this something that you would be interested in doing? Otherwise, I can try some experiments to see how it looks. -Chris. I'm going send the webrev set shortly. With kind regards, Ivan On 3/23/18 10:18 AM, Chris Hegarty wrote: Given that JDK-8058965 removed the IPv6 support from TwoStacksPlainSocketImpl, the native socketListen method no longer needs to deal with the non-IPv4 code path. This simplifies the code, brings it in line with the unix variants, and removes a possible problematic code path that could close the socket without notifying the socket cleaner. http://cr.openjdk.java.net/~chegar/8200181.00/ https://bugs.openjdk.java.net/browse/JDK-8200181 -Chris. [1] https://bugs.openjdk.java.net/browse/JDK-8058965 -- With kind regards, Ivan Gerasimov
Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]
Hello! I reverted the direction of the patch, and now TwoStacks implementation is changed to mimic DualStack. As part of this fix, I also added another @run line to several tests to execute them with the option -Djava.net.preferIPv4Stack=true to make sure both implementations work as expected. After this patch the diff shows difference in only hundred lines of the two implementations, so it will be easy to merge them together on the next step. Here's the set of webrev: WEBREV-000: http://cr.openjdk.java.net/~igerasim/8198358/02/000/webrev/ remove overridden TwoStacksPlainSocketImpl.close(). The only difference from the base AbstractPlainSocketImpl was that the base first calls socketPreClose() and then socketClose(), and there's no difference is these two on Windows. WEBREV-001: http://cr.openjdk.java.net/~igerasim/8198358/02/001/webrev/ rename initProto to initIDs WEBREV-002: http://cr.openjdk.java.net/~igerasim/8198358/02/002/webrev/ changing socketBind WEBREV-003: http://cr.openjdk.java.net/~igerasim/8198358/02/003/webrev/ changing socketCreate and socketListen WEBREV-004: http://cr.openjdk.java.net/~igerasim/8198358/02/004/webrev/ changing socketAvailable WEBREV-005: http://cr.openjdk.java.net/~igerasim/8198358/02/005/webrev/ changing socketClose0 WEBREV-006: http://cr.openjdk.java.net/~igerasim/8198358/02/006/webrev/ changing socketShutdown WEBREV-007: http://cr.openjdk.java.net/~igerasim/8198358/02/007/webrev/ changing socketSendUrgentData WEBREV-008: http://cr.openjdk.java.net/~igerasim/8198358/02/008/webrev/ changing socketAccept WEBREV-009: http://cr.openjdk.java.net/~igerasim/8198358/02/009/webrev/ changing socketConnect WEBREV-010: http://cr.openjdk.java.net/~igerasim/8198358/02/010/webrev/ delegating to super.getOption(), overriding socketGetOption WEBREV-011: http://cr.openjdk.java.net/~igerasim/8198358/02/011/webrev/ changing socketSetOption WEBREV-012: http://cr.openjdk.java.net/~igerasim/8198358/02/012/webrev/ changing socketGetOption WEBREV-013: http://cr.openjdk.java.net/~igerasim/8198358/02/013/webrev/ removing unnecessary global variables WEBREV-014: http://cr.openjdk.java.net/~igerasim/8198358/02/014/webrev/ network tests to run with -Djava.net.preferIPv4Stack=true For reference, here's the combined webrev: http://cr.openjdk.java.net/~igerasim/8198358/03/webrev/ With kind regards, Ivan On 3/9/18 3:50 AM, Chris Hegarty wrote: Ivan, Thanks for breaking up the changes, it makes it easier to review. I disagree with changing DualStackPlainSocketImp to conform with TwoStacks (and unix/PlainSocketImpl). I would prefer to move the latter to the former. Specifically, around the use of fdAccess. -Chris. On 06/03/18 20:31, Ivan Gerasimov wrote: In order to make is easier to review the fix, I made the webrev.ksh to generate a series of incremental webrevs from the mq patch stack. Here's the list of the incremental changes with a brief comments: WEBREV-000: http://cr.openjdk.java.net/~igerasim/8198358/00/000/webrev/ Only changing the order of methods declaration WEBREV-001: http://cr.openjdk.java.net/~igerasim/8198358/00/001/webrev/ Renaming initIDs to initProto WEBREV-002: http://cr.openjdk.java.net/~igerasim/8198358/00/002/webrev/ Changing socketBind WEBREV-003: http://cr.openjdk.java.net/~igerasim/8198358/00/003/webrev/ Changing socketCreate WEBREV-004: http://cr.openjdk.java.net/~igerasim/8198358/00/004/webrev/ Changing socketListen WEBREV-005: http://cr.openjdk.java.net/~igerasim/8198358/00/005/webrev/ Changin socketAvailable WEBREV-006: http://cr.openjdk.java.net/~igerasim/8198358/00/006/webrev/ Changing socketClose0 WEBREV-007: http://cr.openjdk.java.net/~igerasim/8198358/00/007/webrev/ Changing socketShutdown WEBREV-008: http://cr.openjdk.java.net/~igerasim/8198358/00/008/webrev/ Changing socketSendUrgentData WEBREV-009: http://cr.openjdk.java.net/~igerasim/8198358/00/009/webrev/ Changing socketAccept WEBREV-010: http://cr.openjdk.java.net/~igerasim/8198358/00/010/webrev/ Changing socketConnect WEBREV-011: http://cr.openjdk.java.net/~igerasim/8198358/00/011/webrev/ Minor editing, comments, moving code WEBREV-012: http://cr.openjdk.java.net/~igerasim/8198358/00/012/webrev/ Changing socketSetOption WEBREV-013: http://cr.openjdk.java.net/~igerasim/8198358/00/013/webrev/ Changing socketGetOption WEBREV-014: http://cr.openjdk.java.net/~igerasim/8198358/00/014/webrev/ Moving a few methods one more time Accumulative webrev with all the changes above is available here: http://cr.openjdk.java.net/~igerasim/8198358/01/webrev/ Thanks in advance! Ivan On 3/1/18 8:43 PM, Ivan Gerasimov wrote: Hello! I'd like to do the next step toward removing the TwoStacks socket implementation on Windows. It would be aligning the two implementations (DualStack and TwoStacks), so they can be easier merged together during the next step. There are three PlainSocketImpl
Re: RFR 8200181 [11] Remove superflous non-IPv4 code from Java_java_net_TwoStacksPlainSocketImpl_socketListen
Hi Chris! I was about to submit a patch for 8198358 that heavily modifies TwoStacksPlainSocketImpl to make it aligned with DualStackPlainSocketImpl. That patch will also remove this problematic code. I can rebase my patch, but wouldn't it be easier to proceed with that fix? I'm going send the webrev set shortly. With kind regards, Ivan On 3/23/18 10:18 AM, Chris Hegarty wrote: Given that JDK-8058965 removed the IPv6 support from TwoStacksPlainSocketImpl, the native socketListen method no longer needs to deal with the non-IPv4 code path. This simplifies the code, brings it in line with the unix variants, and removes a possible problematic code path that could close the socket without notifying the socket cleaner. http://cr.openjdk.java.net/~chegar/8200181.00/ https://bugs.openjdk.java.net/browse/JDK-8200181 -Chris. [1] https://bugs.openjdk.java.net/browse/JDK-8058965 -- With kind regards, Ivan Gerasimov
Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]
Hi Chris! Thank you for reviewing the patch set! On 3/9/18 3:50 AM, Chris Hegarty wrote: Ivan, Thanks for breaking up the changes, it makes it easier to review. I disagree with changing DualStackPlainSocketImp to conform with TwoStacks (and unix/PlainSocketImpl). I would prefer to move the latter to the former. Specifically, around the use of fdAccess. Do you think unix/PlainSocketImpl should also be refactored that way at some point later? It looks tempting to make Windows and Unix implementation similar, so that any further modifications should they need to be done to both will be easier to apply. That was the primary reason why I choose TwoStacks as the basis and aligned DualStack with it. It can surely be done the opposite way, though I want to make sure it is the preferable way to go. With kind regards, Ivan -Chris. On 06/03/18 20:31, Ivan Gerasimov wrote: In order to make is easier to review the fix, I made the webrev.ksh to generate a series of incremental webrevs from the mq patch stack. Here's the list of the incremental changes with a brief comments: WEBREV-000: http://cr.openjdk.java.net/~igerasim/8198358/00/000/webrev/ Only changing the order of methods declaration WEBREV-001: http://cr.openjdk.java.net/~igerasim/8198358/00/001/webrev/ Renaming initIDs to initProto WEBREV-002: http://cr.openjdk.java.net/~igerasim/8198358/00/002/webrev/ Changing socketBind WEBREV-003: http://cr.openjdk.java.net/~igerasim/8198358/00/003/webrev/ Changing socketCreate WEBREV-004: http://cr.openjdk.java.net/~igerasim/8198358/00/004/webrev/ Changing socketListen WEBREV-005: http://cr.openjdk.java.net/~igerasim/8198358/00/005/webrev/ Changin socketAvailable WEBREV-006: http://cr.openjdk.java.net/~igerasim/8198358/00/006/webrev/ Changing socketClose0 WEBREV-007: http://cr.openjdk.java.net/~igerasim/8198358/00/007/webrev/ Changing socketShutdown WEBREV-008: http://cr.openjdk.java.net/~igerasim/8198358/00/008/webrev/ Changing socketSendUrgentData WEBREV-009: http://cr.openjdk.java.net/~igerasim/8198358/00/009/webrev/ Changing socketAccept WEBREV-010: http://cr.openjdk.java.net/~igerasim/8198358/00/010/webrev/ Changing socketConnect WEBREV-011: http://cr.openjdk.java.net/~igerasim/8198358/00/011/webrev/ Minor editing, comments, moving code WEBREV-012: http://cr.openjdk.java.net/~igerasim/8198358/00/012/webrev/ Changing socketSetOption WEBREV-013: http://cr.openjdk.java.net/~igerasim/8198358/00/013/webrev/ Changing socketGetOption WEBREV-014: http://cr.openjdk.java.net/~igerasim/8198358/00/014/webrev/ Moving a few methods one more time Accumulative webrev with all the changes above is available here: http://cr.openjdk.java.net/~igerasim/8198358/01/webrev/ Thanks in advance! Ivan On 3/1/18 8:43 PM, Ivan Gerasimov wrote: Hello! I'd like to do the next step toward removing the TwoStacks socket implementation on Windows. It would be aligning the two implementations (DualStack and TwoStacks), so they can be easier merged together during the next step. There are three PlainSocketImpl implementations in JDK: java.base/windows/classes/java/net/DualStackPlainSocketImpl.java java.base/windows/classes/java/net/TwoStacksPlainSocketImpl.java java.base/unix/classes/java/net/PlainSocketImpl.java While two later have very similar organization (in particular, set of native methods), the former is organized slightly differently. In order to merge the two Windows implementation together, they first need to be organized in a similar way. For consistency, DualStack implementation will be reorganized to be aligned with TwoStacks and unix/PlainSocketImpl. Bug: https://bugs.openjdk.java.net/browse/JDK-8198358 Webrev: http://cr.openjdk.java.net/~igerasim/8198358/00/webrev/ The change looks somewhat messy, but in fact it was a series of incremental changes, which I still keep in the mercurial 'mq'. (I wish the webrev could be made incremental based on the mq patches, to make it easier to review.) The patched JDK builds fine and all the regression tests pass Okay. Thanks in advance! -- With kind regards, Ivan Gerasimov
Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]
In order to make is easier to review the fix, I made the webrev.ksh to generate a series of incremental webrevs from the mq patch stack. Here's the list of the incremental changes with a brief comments: WEBREV-000: http://cr.openjdk.java.net/~igerasim/8198358/00/000/webrev/ Only changing the order of methods declaration WEBREV-001: http://cr.openjdk.java.net/~igerasim/8198358/00/001/webrev/ Renaming initIDs to initProto WEBREV-002: http://cr.openjdk.java.net/~igerasim/8198358/00/002/webrev/ Changing socketBind WEBREV-003: http://cr.openjdk.java.net/~igerasim/8198358/00/003/webrev/ Changing socketCreate WEBREV-004: http://cr.openjdk.java.net/~igerasim/8198358/00/004/webrev/ Changing socketListen WEBREV-005: http://cr.openjdk.java.net/~igerasim/8198358/00/005/webrev/ Changin socketAvailable WEBREV-006: http://cr.openjdk.java.net/~igerasim/8198358/00/006/webrev/ Changing socketClose0 WEBREV-007: http://cr.openjdk.java.net/~igerasim/8198358/00/007/webrev/ Changing socketShutdown WEBREV-008: http://cr.openjdk.java.net/~igerasim/8198358/00/008/webrev/ Changing socketSendUrgentData WEBREV-009: http://cr.openjdk.java.net/~igerasim/8198358/00/009/webrev/ Changing socketAccept WEBREV-010: http://cr.openjdk.java.net/~igerasim/8198358/00/010/webrev/ Changing socketConnect WEBREV-011: http://cr.openjdk.java.net/~igerasim/8198358/00/011/webrev/ Minor editing, comments, moving code WEBREV-012: http://cr.openjdk.java.net/~igerasim/8198358/00/012/webrev/ Changing socketSetOption WEBREV-013: http://cr.openjdk.java.net/~igerasim/8198358/00/013/webrev/ Changing socketGetOption WEBREV-014: http://cr.openjdk.java.net/~igerasim/8198358/00/014/webrev/ Moving a few methods one more time Accumulative webrev with all the changes above is available here: http://cr.openjdk.java.net/~igerasim/8198358/01/webrev/ Thanks in advance! Ivan On 3/1/18 8:43 PM, Ivan Gerasimov wrote: Hello! I'd like to do the next step toward removing the TwoStacks socket implementation on Windows. It would be aligning the two implementations (DualStack and TwoStacks), so they can be easier merged together during the next step. There are three PlainSocketImpl implementations in JDK: java.base/windows/classes/java/net/DualStackPlainSocketImpl.java java.base/windows/classes/java/net/TwoStacksPlainSocketImpl.java java.base/unix/classes/java/net/PlainSocketImpl.java While two later have very similar organization (in particular, set of native methods), the former is organized slightly differently. In order to merge the two Windows implementation together, they first need to be organized in a similar way. For consistency, DualStack implementation will be reorganized to be aligned with TwoStacks and unix/PlainSocketImpl. Bug: https://bugs.openjdk.java.net/browse/JDK-8198358 Webrev: http://cr.openjdk.java.net/~igerasim/8198358/00/webrev/ The change looks somewhat messy, but in fact it was a series of incremental changes, which I still keep in the mercurial 'mq'. (I wish the webrev could be made incremental based on the mq patches, to make it easier to review.) The patched JDK builds fine and all the regression tests pass Okay. Thanks in advance! -- With kind regards, Ivan Gerasimov
Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]
Thank you Christoph for reviewing this! On 3/6/18 4:53 AM, Langer, Christoph wrote: Hi Ivan, I went through the changes a bit and I would think it is really a good cleanup and will make the future merge of the implementations easier. One thing I saw was that TwoStacksPlainSocketImpl.c has an #include in line 25. I think that can be removed!? Certainly! Probably it's a leftover from already removed code. Thanks for spotting this. With kind regards, Ivan I had problems importing the patch(set) via mercurial queues - but maybe it is an issue of my local mercurial version. Best regards Christoph -Original Message- From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Ivan Gerasimov Sent: Freitag, 2. März 2018 05:43 To: Chris Hegarty <chris.hega...@oracle.com>; net-dev@openjdk.java.net Subject: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win] Hello! I'd like to do the next step toward removing the TwoStacks socket implementation on Windows. It would be aligning the two implementations (DualStack and TwoStacks), so they can be easier merged together during the next step. There are three PlainSocketImpl implementations in JDK: java.base/windows/classes/java/net/DualStackPlainSocketImpl.java java.base/windows/classes/java/net/TwoStacksPlainSocketImpl.java java.base/unix/classes/java/net/PlainSocketImpl.java While two later have very similar organization (in particular, set of native methods), the former is organized slightly differently. In order to merge the two Windows implementation together, they first need to be organized in a similar way. For consistency, DualStack implementation will be reorganized to be aligned with TwoStacks and unix/PlainSocketImpl. Bug: https://bugs.openjdk.java.net/browse/JDK-8198358 Webrev: http://cr.openjdk.java.net/~igerasim/8198358/00/webrev/ The change looks somewhat messy, but in fact it was a series of incremental changes, which I still keep in the mercurial 'mq'. (I wish the webrev could be made incremental based on the mq patches, to make it easier to review.) The patched JDK builds fine and all the regression tests pass Okay. Thanks in advance! -- With kind regards, Ivan Gerasimov -- With kind regards, Ivan Gerasimov
RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]
Hello! I'd like to do the next step toward removing the TwoStacks socket implementation on Windows. It would be aligning the two implementations (DualStack and TwoStacks), so they can be easier merged together during the next step. There are three PlainSocketImpl implementations in JDK: java.base/windows/classes/java/net/DualStackPlainSocketImpl.java java.base/windows/classes/java/net/TwoStacksPlainSocketImpl.java java.base/unix/classes/java/net/PlainSocketImpl.java While two later have very similar organization (in particular, set of native methods), the former is organized slightly differently. In order to merge the two Windows implementation together, they first need to be organized in a similar way. For consistency, DualStack implementation will be reorganized to be aligned with TwoStacks and unix/PlainSocketImpl. Bug: https://bugs.openjdk.java.net/browse/JDK-8198358 Webrev: http://cr.openjdk.java.net/~igerasim/8198358/00/webrev/ The change looks somewhat messy, but in fact it was a series of incremental changes, which I still keep in the mercurial 'mq'. (I wish the webrev could be made incremental based on the mq patches, to make it easier to review.) The patched JDK builds fine and all the regression tests pass Okay. Thanks in advance! -- With kind regards, Ivan Gerasimov
RFR [11] 8058965 : Remove IPv6 support from TwoStacksPlainSocketImpl [win]
Hello! On Windows we have two different implementations of java.net.Socket: DualStackPlainSocketImpl and TwoStacksPlainSocketImpl. Former is used by default, and the later is only used when the system option java.net.preferIPv4Stack is set to "true". In particular, this means that TwoStacksPlainSocketImpl is never used for IPv6, which effectively makes all the IPv6-related code in this class dead. Let's remove this dead code! The proposed patch mostly consists of code removals. The changes were verified by existing regression tests: No failures noticed. I do not propose to change the file name for two reasons: 1) simplifying the review, 2) there are plans for further refactoring in this area, which will include the file renaming. Would you please help review the fix? Bug: https://bugs.openjdk.java.net/browse/JDK-8058965 Webrev: http://cr.openjdk.java.net/~igerasim/8058965/00/webrev/ Thanks in advance! -- With kind regards, Ivan Gerasimov
[jdk10] (XXS) RFR 8189631 : Missing space in the javadoc for InetAddress.createNameService()
Hello!
A typo in the javadoc:
--- a/src/java.base/share/classes/java/net/InetAddress.java
+++ b/src/java.base/share/classes/java/net/InetAddress.java
@@ -1133,7 +1133,7 @@
/**
* Create an instance of the NameService interface based on
- * the setting of the {@codejdk.net.hosts.file} system property.
+ * the setting of the {@code jdk.net.hosts.file} system property.
*
* The default NameService is the PlatformNameService, which
typically
* delegates name and address resolution calls to the underlying
Would you approve the fix?
--
With kind regards,
Ivan Gerasimov
Re: RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses
Thank you Roger for review! On 9/25/17 11:47 AM, Roger Riggs wrote: Hi Ivan, Looks ok to me. I don't see a reason to skimp on the additional size since it is allocated and then freed immediately. The increment might as well be as big as the initial size. Right. Let's use the same value of 15K, which reduces the number of variables to operate with. I don't see a reason to retry if the buffer gets close to ULONG_MAX; I'd just break the for loop and let it fail. (but the new code is just doing what the old code did and retry even if the buffer did not increase). If len is close to ULONG_MAX, it is still larger value of len returned by the previous call to GetAdaptersAddresses (otherwise we wouldn't have gotten ERROR_BUFFER_OVERFLOW.) So, if we're in the loop, there's no way the buffer will not increase (unless there's a failure due to OOM, of course.) Also, please introduce a constant for the number of retries; it will make it clearer and not need to hard code it in two places. Done! Would you please take a look at the updated webrev: http://cr.openjdk.java.net/~igerasim/8187658/01/webrev/ With kind regards, Ivan (I would probably have coded the retry count counting down to zero but its the same effect). Regards, Roger On 9/25/2017 1:03 PM, Ivan Gerasimov wrote: Ping. Please review the proposed change at your convenience. The fix will greatly reduce the possibility of a need to reallocate the buffer to retrieve the results (something that the documentation strongly suggests to avoid), and, if such reallocation still occurs to be necessary, will increase number of retries. With kind regards, Ivan On 9/19/17 10:50 AM, Ivan Gerasimov wrote: Hello! When retrieving information about network interfaces on Windows we make up to 2 attempts to call GetAdaptersAddresses(). It was reported that in very rare cases it may not be sufficient, and even the second attempt can fail with ERROR_BUFFER_OVERFLOW. I suggest that we follow the recommendation given in MSDN [1]: increase the initial buffer size to 15K and do up to 3 attempts to call GetAdaptersAddresses(). BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658 WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/ Would you please help review the fix? [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx -- With kind regards, Ivan Gerasimov
Re: RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses
Ping. Please review the proposed change at your convenience. The fix will greatly reduce the possibility of a need to reallocate the buffer to retrieve the results (something that the documentation strongly suggests to avoid), and, if such reallocation still occurs to be necessary, will increase number of retries. With kind regards, Ivan On 9/19/17 10:50 AM, Ivan Gerasimov wrote: Hello! When retrieving information about network interfaces on Windows we make up to 2 attempts to call GetAdaptersAddresses(). It was reported that in very rare cases it may not be sufficient, and even the second attempt can fail with ERROR_BUFFER_OVERFLOW. I suggest that we follow the recommendation given in MSDN [1]: increase the initial buffer size to 15K and do up to 3 attempts to call GetAdaptersAddresses(). BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658 WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/ Would you please help review the fix? [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx -- With kind regards, Ivan Gerasimov
RFR[10]: 8187658: Bigger buffer for GetAdaptersAddresses
Hello! When retrieving information about network interfaces on Windows we make up to 2 attempts to call GetAdaptersAddresses(). It was reported that in very rare cases it may not be sufficient, and even the second attempt can fail with ERROR_BUFFER_OVERFLOW. I suggest that we follow the recommendation given in MSDN [1]: increase the initial buffer size to 15K and do up to 3 attempts to call GetAdaptersAddresses(). BUGURL: https://bugs.openjdk.java.net/browse/JDK-8187658 WEBREV: http://cr.openjdk.java.net/~igerasim/8187658/00/webrev/ Would you please help review the fix? [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa365915(v=vs.85).aspx -- With kind regards, Ivan Gerasimov
Re: [8u-dev] Request for Review & Approval to backport: 8073542: File Leak in jdk/src/java/base/unix/native/libnet/PlainDatagramSocketImpl.c
Could someone please help review this simple backport? Thanks in advance, Ivan On 29.05.2016 21:08, Ivan Gerasimov wrote: As this has to be re-reviewed, sending the request to net-dev list as well. On 27.05.2016 16:46, Ivan Gerasimov wrote: Hello! I'd like to backport the fix to jdk8u-dev. The fix had to be slightly adjusted due to the different way the error messages are retrieved in jdk8. Here's the webrev with modified fix: http://cr.openjdk.java.net/~igerasim/8073542/00/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8073542 Jdk9 changeset: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/bb6e5a409fef Jdk9 review: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035223.html With kind regards, Ivan
[8u-dev] Request for Review & Approval to backport: 8073542: File Leak in jdk/src/java/base/unix/native/libnet/PlainDatagramSocketImpl.c
As this has to be re-reviewed, sending the request to net-dev list as well. On 27.05.2016 16:46, Ivan Gerasimov wrote: Hello! I'd like to backport the fix to jdk8u-dev. The fix had to be slightly adjusted due to the different way the error messages are retrieved in jdk8. Here's the webrev with modified fix: http://cr.openjdk.java.net/~igerasim/8073542/00/webrev/ Bug: https://bugs.openjdk.java.net/browse/JDK-8073542 Jdk9 changeset: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/bb6e5a409fef Jdk9 review: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035223.html With kind regards, Ivan
Re: [PING] RFR: JDK-8135259: InetAddress.getAllByName only reports "unknown error" instead of actual cause
Hi Ramanand! Your fix looks good to me. I'm forwarding your request to net-dev@openjdk.java.net which is probably more appropriate to review this fix. It would be good, if net-dev people can confirm they're Okay with the fix. Sincerely yours, Ivan On 10.02.2016 10:08, Ramanand Patil wrote: Hi all, Please help me in getting this review done. Regards, Ramanand. -Original Message- From: Ramanand Patil Sent: Friday, February 05, 2016 10:46 PM To: core-libs-...@openjdk.java.net Subject: RFR: JDK-8135259: InetAddress.getAllByName only reports "unknown error" instead of actual cause Hi all Please review the fix for bug: https://bugs.openjdk.java.net/browse/JDK-8135259 Bug Description: Attempts to resolve a host unknown to the DNS server cause an UnknownHostException stating "unknown error" instead of "Name or service not known". Webrev: http://cr.openjdk.java.net/~rpatil/8135259/webrev.00/ Fix: Using a function call directly "gai_strerror(gai_error)" instead of using the function pointer which was declared but not initialized. Apart from this I have removed few other unused variables and function pointers. Brief description of how this bug was introduced is added to the bug comment. Regards, Ramanand.
Re: RFR 8139373 : [TEST_BUG] java/net/MulticastSocket/MultiDead.java failed with timeout
Thank you Chris for review and proofreading! :-) Sincerely yours, Ivan On 21.10.2015 15:21, Chris Hegarty wrote: Looks fine Ivan, Just a typo on: 48 Utils.adjustTimeout(CHILDREN_COUNT * CHILD_TIMEOT * 2); -Chris. On 21/10/15 11:06, Ivan Gerasimov wrote: Hello! A few failures of the recently added regtest were observed. The failures seem to be due to slow machines. The suggested fix is to 1) increase the timeout, 2) take into account the timeout factor from the jtreg's settings, 3) measure the time of individual cycle of the loop, and give up, if it appeared to be too slow. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8139373 WEBREV: http://cr.openjdk.java.net/~igerasim/8139373/00/webrev/ Would you help review this fix? Sincerely yours, Ivan
RFR 8139373 : [TEST_BUG] java/net/MulticastSocket/MultiDead.java failed with timeout
Hello! A few failures of the recently added regtest were observed. The failures seem to be due to slow machines. The suggested fix is to 1) increase the timeout, 2) take into account the timeout factor from the jtreg's settings, 3) measure the time of individual cycle of the loop, and give up, if it appeared to be too slow. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8139373 WEBREV: http://cr.openjdk.java.net/~igerasim/8139373/00/webrev/ Would you help review this fix? Sincerely yours, Ivan
Re: RFR 8073542 : File Leak in jdk/src/java/base/unix/native/libnet/PlainDatagramSocketImpl.c
Hi Vyom! The change looks fine, thanks. You've used strerr() to extract the error string, so it may be good to coordinate your patch with the fix for JDK-8133249. Robert (CCed) is working on it at the moment. Sincerely yours, Ivan On 16.09.2015 12:08, Vyom Tewari wrote: Hi All, Please review my changes for below bug. Bug: JDK-8073542 : File Leak in jdk/src/java/base/unix/native/libnet/PlainDatagramSocketImpl.c Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8073542/webrev.00 This change ensure that if "setsocketopt" fails we close the corresponding fd and throw correct exception. Thanks, Vyom
[8u-dev] Request for review and for approval to backport: 8072466: Deadlock when initializing MulticastSocket and DatagramSocket
Hello! I'd like to backport this recent fix from jdk9 to jdk8u-dev. The fix applies *almost* cleanly after unshuffling. The only manual editing I had to do was creating Java_java_net_DualStackPlainDatagramSocketImpl_initIDs() function in DualStackPlainDatagramSocketImpl.c, which didn't exist in jdk8. All the other changes are the same as in jdk9. Bug: https://bugs.openjdk.java.net/browse/JDK-8072466 Jdk9 change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/3884ca98c792 Jdk9 review: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035053.html Jdk8 webrev: http://cr.openjdk.java.net/~igerasim/8072466/01/webrev/ Would you please review/approve it? Sincerely yours, Ivan
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Hi! The close() function isn't really restartable. So, I think, it's more correct to replace RESTARTABLE(close(s), res); with res = close(s); Sincerely yours, Ivan On 07.09.2015 12:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. Thanks, Vyom
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Thanks! It looks good to me now. Sincerely yours, Ivan On 07.09.2015 16:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ Thanks, Vyom On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
Re: RFR 8072466: Deadlock when starting MulticastSocket and DatagramSocket
Thank you Chris for the review! Sincerely yours, Ivan On 07.09.2015 17:39, Chris Hegarty wrote: This looks like a nice cleanup, and fix. Thanks Ivan. -Chris. On 05/09/15 15:25, Ivan Gerasimov wrote: Hi everyone! The fix didn't bring enough attention back in February for some reason. So, I'd like to re-request a review. I've added a regression test, which reliably reproduces a deadlock on my local Windows 7 machine. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072466 WEBREV: http://cr.openjdk.java.net/~igerasim/8072466/00/webrev/ Sincerely yours, Ivan On 17.02.2015 9:54, Ivan Gerasimov wrote: Can someone help review this fix please? Comments/suggestions are welcome! Sincerely yours, Ivan On 06.02.2015 20:48, Ivan Gerasimov wrote: Hello! There has been a deadlock in jdk-net code noticed on Windows. In the bug description there's a stack snippet showing that one of the deadlocked threads stuck in the native initialization code of DualStackPlainDatagramSocketImpl and the other -- in initializing of TwoStacksPlainDatagramSocketImpl. I suspect that the reason might be due to unordered initialization: AbstractPlainDatagramSocketImpl, the parent of both classes above, explicitly loads TwoStacksPlainDatagramSocketImpl from the native init(). Thus, when both classes are being initialized concurrently, it may end with a clash. Another issue noticed by Mark Sheppard is that the Windows version of DefaultDatagramSocketImplFactory.createDatagramSocketImpl() isn't synchronized, but reads/modifies shared data. Thus, I removed modification and declared all the accessed fields final. Would you please help review the proposed fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072466 WEBREV: http://cr.openjdk.java.net/~igerasim/8072466/0/webrev/ The build went fine on all the platforms, all the tests from (net|nio|io) passed Okay. Sincerely yours, Ivan
Re: RFR 8072466: Deadlock when starting MulticastSocket and DatagramSocket
Hi everyone! The fix didn't bring enough attention back in February for some reason. So, I'd like to re-request a review. I've added a regression test, which reliably reproduces a deadlock on my local Windows 7 machine. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072466 WEBREV: http://cr.openjdk.java.net/~igerasim/8072466/00/webrev/ Sincerely yours, Ivan On 17.02.2015 9:54, Ivan Gerasimov wrote: Can someone help review this fix please? Comments/suggestions are welcome! Sincerely yours, Ivan On 06.02.2015 20:48, Ivan Gerasimov wrote: Hello! There has been a deadlock in jdk-net code noticed on Windows. In the bug description there's a stack snippet showing that one of the deadlocked threads stuck in the native initialization code of DualStackPlainDatagramSocketImpl and the other -- in initializing of TwoStacksPlainDatagramSocketImpl. I suspect that the reason might be due to unordered initialization: AbstractPlainDatagramSocketImpl, the parent of both classes above, explicitly loads TwoStacksPlainDatagramSocketImpl from the native init(). Thus, when both classes are being initialized concurrently, it may end with a clash. Another issue noticed by Mark Sheppard is that the Windows version of DefaultDatagramSocketImplFactory.createDatagramSocketImpl() isn't synchronized, but reads/modifies shared data. Thus, I removed modification and declared all the accessed fields final. Would you please help review the proposed fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072466 WEBREV: http://cr.openjdk.java.net/~igerasim/8072466/0/webrev/ The build went fine on all the platforms, all the tests from (net|nio|io) passed Okay. Sincerely yours, Ivan
Re: RFR 8072466: Deadlock when starting MulticastSocket and DatagramSocket
Can someone help review this fix please? Comments/suggestions are welcome! Sincerely yours, Ivan On 06.02.2015 20:48, Ivan Gerasimov wrote: Hello! There has been a deadlock in jdk-net code noticed on Windows. In the bug description there's a stack snippet showing that one of the deadlocked threads stuck in the native initialization code of DualStackPlainDatagramSocketImpl and the other -- in initializing of TwoStacksPlainDatagramSocketImpl. I suspect that the reason might be due to unordered initialization: AbstractPlainDatagramSocketImpl, the parent of both classes above, explicitly loads TwoStacksPlainDatagramSocketImpl from the native init(). Thus, when both classes are being initialized concurrently, it may end with a clash. Another issue noticed by Mark Sheppard is that the Windows version of DefaultDatagramSocketImplFactory.createDatagramSocketImpl() isn't synchronized, but reads/modifies shared data. Thus, I removed modification and declared all the accessed fields final. Would you please help review the proposed fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072466 WEBREV: http://cr.openjdk.java.net/~igerasim/8072466/0/webrev/ The build went fine on all the platforms, all the tests from (net|nio|io) passed Okay. Sincerely yours, Ivan
RFR 8072466: Deadlock when starting MulticastSocket and DatagramSocket
Hello! There has been a deadlock in jdk-net code noticed on Windows. In the bug description there's a stack snippet showing that one of the deadlocked threads stuck in the native initialization code of DualStackPlainDatagramSocketImpl and the other -- in initializing of TwoStacksPlainDatagramSocketImpl. I suspect that the reason might be due to unordered initialization: AbstractPlainDatagramSocketImpl, the parent of both classes above, explicitly loads TwoStacksPlainDatagramSocketImpl from the native init(). Thus, when both classes are being initialized concurrently, it may end with a clash. Another issue noticed by Mark Sheppard is that the Windows version of DefaultDatagramSocketImplFactory.createDatagramSocketImpl() isn't synchronized, but reads/modifies shared data. Thus, I removed modification and declared all the accessed fields final. Would you please help review the proposed fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8072466 WEBREV: http://cr.openjdk.java.net/~igerasim/8072466/0/webrev/ The build went fine on all the platforms, all the tests from (net|nio|io) passed Okay. Sincerely yours, Ivan
Re: RFR 8067105: Socket returned by ServerSocket.accept() is inherited by child process on Windows
Hi Chris! I think it's better to pass the last argument to SetHandleInformation as 0 rather than FALSE. This argument is DWORD flag, which is a bit subset of the second argument. Sincerely yours, Ivan On 28.01.2015 23:01, Chris Hegarty wrote: Reviving an old code review [1], after further investigation… Pertinent details from previous review: A socket connection which is returned by ServerSocket.accept() is inherited by a child process. The expected behavior is that the socket connection is not inherited by the child process. This is an oversight in the original implementation, that only sets HANDLE_FLAG_INHERIT for newly created sockets. The native socket returned by ServerSocket.accept() should be configured so it will not be inherited by a child process, SetHandleInformation(HANDLE, HANDLE_FLAG_INHERIT, FALSE). http://cr.openjdk.java.net/~chegar/8067105/webrev.00/webrev/ http://cr.openjdk.java.net/%7Echegar/8067105/webrev.00/webrev/ — There were on objections to the changes, since they are mainly benign, but I took the action to investigate why we are calling CreateProcess with bInheritHandles set to TRUE. It appears that, without major work, we cannot change this. From 7147084 [2]: Java does not provide the API to change inherit-bit for any handle. More over, since at least the JDK 6, it is assumed that all Java-created handles have no installed inherit-bit . The only handles that change the inherit-bit to 1 in the Java call are the handles of redirected Input, Output, and Error streams (IOE streams for short) for child process. That is the way these redirect the streams work. That's why we can not give up the nomination in [TRUE] the parameter [bInheritHandles] in the [CreateProcess] call. And I want to mention again that this is the only place in JDK where Java installs the inherit-bit. Java itself does not use handle inheritance. — Ivan pointed out that HANDLE_FLAG_INHERIT will not always work, in the case of Layered Service Providers, see [3], but it will work in the standard case. Finally, I think that we will need to revisit the process creation implementation at some point, to see if bInheritHandles can be set to FALSE, but that is a larger, more significant, piece of work, that should be done separately. -Chris. P.S. if there are no objections to the changes I will amend an existing test case to cover these new cases. [1] http://mail.openjdk.java.net/pipermail/net-dev/2014-December/008789.html [2] https://bugs.openjdk.java.net/browse/JDK-7147084?focusedCommentId=13322689page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13322689 [3] http://stackoverflow.com/questions/12058911/can-tcp-socket-handles-be-set-not-inheritable
Re: RFR(M): 8060470 : Unify and simplify the implmentations of Inet{4, 6}AddressImpl_getLocalHostName()
On 27.10.2014 21:45, Volker Simonis wrote:
On Fri, Oct 24, 2014 at 7:07 PM, Ivan Gerasimov
ivan.gerasi...@oracle.com wrote:
Hi Volker!
I'm not a Reviewer, but have a couple of minor comments.
In the C source files you changed the indentation to two spaces.
It looks inconsistent with other JDK sources.
I know that in hotspot they use two space indentation, but it's a different
set of sources.
Well, the problem is that already that very file contains code in both
code conventions (see for example the implementations of 'ping4()'
and 'Java_java_net_Inet4AddressImpl_isReachable0()' in
Inet4AddressImpl.c which are mostly indented by two spaces). I have no
problems to adhere to any convention as long as it is generally
obeyed. As this does not seemed to be the case in these files, I've
just chosen what I thought is most appropriate. So to keep a long
story short - I can either:
1. indent my changes to four spaces (which will still let the files
with mixed indentation)
2. change all indentation in the file to two spaces
3. change all indentation in the file to four spaces
Please just tell me what you'd prefer.
I think #1 it the most appropriate here.
Inet4AddressImpl.c:
110 jboolean reverseLookup = (*env)-GetStaticBooleanField(env, ia_class,
ia_doIPv4ReverseLookup);
Since doIPv4ReverseLookup never changes, wouldn't it make sense to declare
jboolean reverseLookup static?
This way it would be retrieved only once.
The same in Inet6AddressImpl.c:
66 jboolean reverseLookup = (*env)-GetStaticBooleanField(env, ia_class,
ia_doIPv6ReverseLookup);
And a static value retrieved here:
68 if ((*env)-GetStaticBooleanField(env, ia_class,
ia_preferIPv6AddressID)) {
I think simply declaring the mentioned variables static is not
possible in C and this is a C file compiled with a C compiler.
You would get a initializer element is not constant error (see for
example
http://stackoverflow.com/questions/5921920/difference-between-initialization-of-static-variables-in-c-and-c).
I could of course use a second static varibale to do the
initialization only once, but I think that's not worth it.
Ah, yes, you're right.
I missed the fact it's plain C, so a static variable must be initialized
with a constant.
Thanks,
Volker
Sincerely yours,
Ivan
On 24.10.2014 18:47, Volker Simonis wrote:
Hi,
could somebody please have a quick look at this change.
It's really not that complicated as it looks like from the comments -
I just didn't manage to write it up in a more concise way :)
Thanks,
Volker
On Thu, Oct 16, 2014 at 4:39 PM, Volker Simonis
volker.simo...@gmail.com wrote:
Hi,
could you please hava a look at the following change:
http://cr.openjdk.java.net/~simonis/webrevs/8060470.v1
https://bugs.openjdk.java.net/browse/JDK-8060470
It's probably easier to read the following in the webrev, but I copy
it below for discussion.
Regards,
Volker
So here comes the first version of this change. Its main focus is the
unification and simplification of
Inet{4,6}AddressImpl_getLocalHostName() (notice that these native
methods are currently only called from InetAddress.getLocalHost() so
the impact is manageable):
- Simplification: the current implementation has three versions of
this function: two versions of Inet4AddressImpl_getLocalHostName()
(one for _ALLBSD_SOURCE !HAS_GLIBC_GETHOSTBY_R and another one
for the other Unix versions) and one version of
Inet6AddressImpl_getLocalHostName(). All these functions are very
similar and can be easily factored out into one new method.
- Unification: there are subtle and probably unnecessary differences
between the IPv4 and IPv6 version of these methods which can be easily
eliminated.
The only difference between the two IPv4 versions was the ai_family
flag passed as hint to the getaddrinfo() call. The Mac version used
AF_UNSPEC while the general Unix version used AF_INET. I don't see a
reason (and my tests didn't show any problems) why we couldn't use
AF_INET on MacOS as well.
The IPv6 version used AF_UNSPEC as well. The new refactored method
getLocalHostName() which is now called from both, the single instance
of Inet4AddressImpl_getLocalHostName() and
Inet6AddressImpl_getLocalHostName() uses AF_INET in the IPv4 case
(which shouldn't change anything) and AF_INET6 for the IPv6 case.
Additionally, it uses the flag AI_V4MAPPED in the IPv6 case. This will
return an IPv4-mapped IPv6 addresses if no matching IPv6 addresses
could be found.
The last difference between the old IPv4 and IPv6 versions was the
fact that the IPv4 versions always did a reverse lookup for the host
name. That means that after querying the hostname with gethostname(),
they used a call to getaddrinfo() to get the IP address of the host
name and finally they called getnameinfo() on that IP address to get
the host name once again. The IPv6 version only did this reverse
lookup on Solaris.
It is unclear why this reverse lookup was necessary at all. Especially
if we take into account
RFR 8059450: Not quite correct code sample in javadoc
Hello! This is a javadoc bug. In the code sample a redundant argument to a constructor of URI is passed. Probably a copy-paste error. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8059450 WEBREV: http://cr.openjdk.java.net/~igerasim/8059450/0/webrev/ Sincerely yours, Ivan
Re: RFR 8059450: Not quite correct code sample in javadoc
Thanks Chris! On 01.10.2014 20:49, Chris Hegarty wrote: On 1 Oct 2014, at 01:48, Ivan Gerasimov ivan.gerasi...@oracle.com wrote: Hello! This is a javadoc bug. In the code sample a redundant argument to a constructor of URI is passed. Probably a copy-paste error. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8059450 WEBREV: http://cr.openjdk.java.net/~igerasim/8059450/0/webrev/ This looks fine to me Ivan ( good eyes! ). I tried to use this sample, when figuring out how a URI deals with underscores. So it was noticed not by eyes, but by fingers :-) Sincerely yours, Ivan -Chris. Sincerely yours, Ivan
Re: RFR 8058824: Drop TwoStacks socket implementation in jdk9 [win] - Part I
Thanks Michael! On 23.09.2014 13:41, Michael McMahon wrote: Hi Ivan, Did you look at the possibility of removing the TwoStacks class altogether? For Solaris/Linux etc. ipv4 only and ipv6/v4 are all handled in the same impl class with just a switch at socket creation time, selecting AF_INET or AF_INET6. Yes, trying to merge DualStack with IPv4-only (the remaining part of TwoStacks) was planned as the second step. It's not that straight-forward, so better be done separately from this change. If there is a good reason to keep the implementations separate then I think the class should be renamed to reflect its new purpose. I left it with the old name to make the review simpler. No problem to rename the files, of course. I only thought that if we are planing to combine both implementations, renaming will not matter that much and only make the things more complicated. Sincerely yours, Ivan
RFR 8058965: Remove IPv6 support from TwoStacksPlainSocketImpl
Thank you Michael! Okay. I see the part 1 in the subject line now, but maybe the bug should be updated to define the scope of the change and we should file a separate bug report then. I created the subtask to track this part (the subject of the thread is changed accordingly): https://bugs.openjdk.java.net/browse/JDK-8058965 I'm okay with leaving the class with the current name. What about the datagram socket code? It might make sense to include the equivalent changes there also. TwoStacksPlainDatagramSocketImpl is there for two purposes: for IPv4 only as above and for doing multicasting. DualStackPlainDatagramSocketImpl does not have multicasting implemented (from the comment: This is to overcome the lack of behavior defined for multicasting over a dual layer socket by the RFC I'm going to see if it's possible to implement multicasting over DualStack socket with the current API: http://msdn.microsoft.com/en-us/library/windows/desktop/ms738558(v=vs.85).aspx If it is, it should be possible to drop IPv6 part of TwoStacksPlainDatagramSocketImpl as well. There might need to be some code in net_util_md.c that needs to be removed also (NET_Timeout2() function?) I've just checked its content, and it all is still used somewhere. NET_Timeout2() is called from TwoStacksPlainDatagramSocketImpl. Once we remove/reoraganize it, all the unused functions will be erased too. Sincerely yous, Ivan
RFR 8058824: Drop TwoStacks socket implementation in jdk9 [win] - Part I
Hello! Here is a proposal for the first step in cleaning up the TwoStacks socket implementation. It is proposed to drom IPv6 support in the implementation of TwoStacksPlainSocketImpl class (so formally, it's no more Two Stacks). Therefore, this class will only be used when the user explicitly requests IPv4 only configuration through setting java.net.preferIPv4Stack option. The fix mostly consists of removal, so it should be relatively easy to review. All the jdk_net tests pass. BUGURL: https://bugs.openjdk.java.net/browse/JDK-8058824 WEBREV: http://cr.openjdk.java.net/~igerasim/8058824/0/webrev/ Sincerely yours, Ivan
RFR [7010989]: Duplicate closure of file descriptors leads to unexpected and incorrect closure of sockets
Hello! In the implementation of TwoStack sockets on Windows there's is a possibility of trying to close the sockets twice in the case of a failure. TwoStack implementation isn't the default one, but it still can be used with jdk7u/8u on old platforms. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-7010989 WEBREV: http://cr.openjdk.java.net/~igerasim/7010989/0/webrev/ Sincerely yours, Ivan
Re: RFR: [8036088] - Thread-unsafe strtok() is used to parse
VS compiler issues this warning on strtok() usage: : warning C4996: 'strtok': This function or variable may be unsafe. Consider using strtok_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. So, the suggested fix can reduce number of complains from the compiler. If I had realized that strtok() is thread-safe under Windows, I wouldn't bother anyone with this. But as the fix is already prepared, I think it's better push it, even though it adds only a little. If you guys are OK with the fix, of course. Sincerely yours, Ivan On 04.03.2014 14:28, Chris Hegarty wrote: On 4 Mar 2014, at 03:25, Ivan Gerasimov ivan.gerasi...@oracle.com wrote: Yes, you're right. strtok() is thread-safe in Windows unlike its Unix counterpart. Thus using strtok_s() only adds some boundary checking in this case. Ivan, Are you now withdrawing this request for review? Or are you suggesting that strtok_s should be used anyway? -Chris. Sincerely yours, Ivan On 04.03.2014 0:26, Salter, Thomas A wrote: strtok is thread-safe in MS C/C++. It uses thread-local store to hold its state. strtok_s can be called recursively to parse different strings, though it's named like the MS extensions that check for buffer overruns. http://msdn.microsoft.com/en-us/library/2c8d19sb(v=vs.100).aspx -- Message: 5 Date: Mon, 03 Mar 2014 21:01:15 +0400 From: Ivan Gerasimov ivan.gerasi...@oracle.com Subject: Re: RFR: [8036088] - Thread-unsafe strtok() is used to parse the list of overrides To: Christos Zoulas chris...@zoulas.com,OpenJDK Network Dev list net-dev@openjdk.java.net Message-ID: 5314b55b.9070...@oracle.com Content-Type: text/plain; charset=UTF-8; format=flowed Hi Christos! On 03.03.2014 20:52, chris...@zoulas.com wrote: On Mar 3, 8:32pm, ivan.gerasi...@oracle.com (Ivan Gerasimov) wrote: -- Subject: RFR: [8036088] - Thread-unsafe strtok() is used to parse the list | Hello! | | The strtok() function is used in | ./windows/native/sun/net/spi/DefaultProxySelector.c. | This function is not thread safe, so it may potentially cause a problem. | | The failure in this particular place would be very unlikely, because | this code should be executed only once during initialization. | Therefore, no regtest provided. | | The fix would be to use a thread-safe equivalent, which is strtok_s() | under Windows. | | Would you please help review this simple fix? | | BUGURL: https://bugs.openjdk.java.net/browse/JDK-8036088 | WEBREV: http://cr.openjdk.java.net/~igerasim/8036088/0/webrev/ Doesn't windows have strtok_r() IEEE Std 1003.1c-1995 (``POSIX.1''). MSDN does not refer to strtok_r(). Grepping the JDK code shows that strtok_s() is used in the windows-specific code. Sincerely yours, Ivan christos End of net-dev Digest, Vol 81, Issue 3 **
Re: RFR: [8036088] - Thread-unsafe strtok() is used to parse
Thank you Chris! On 04.03.2014 16:43, Chris Hegarty wrote: On 4 Mar 2014, at 11:52, Ivan Gerasimov ivan.gerasi...@oracle.com wrote: VS compiler issues this warning on strtok() usage: : warning C4996: 'strtok': This function or variable may be unsafe. Consider using strtok_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. So, the suggested fix can reduce number of complains from the compiler. If I had realized that strtok() is thread-safe under Windows, I wouldn't bother anyone with this. But as the fix is already prepared, I think it's better push it, even though it adds only a little. If you guys are OK with the fix, of course. The changes seem mostly benign to me. I have no objections to being listed as a reviewer. -Chris. Sincerely yours, Ivan On 04.03.2014 14:28, Chris Hegarty wrote: On 4 Mar 2014, at 03:25, Ivan Gerasimov ivan.gerasi...@oracle.com wrote: Yes, you're right. strtok() is thread-safe in Windows unlike its Unix counterpart. Thus using strtok_s() only adds some boundary checking in this case. Ivan, Are you now withdrawing this request for review? Or are you suggesting that strtok_s should be used anyway? -Chris. Sincerely yours, Ivan On 04.03.2014 0:26, Salter, Thomas A wrote: strtok is thread-safe in MS C/C++. It uses thread-local store to hold its state. strtok_s can be called recursively to parse different strings, though it's named like the MS extensions that check for buffer overruns. http://msdn.microsoft.com/en-us/library/2c8d19sb(v=vs.100).aspx -- Message: 5 Date: Mon, 03 Mar 2014 21:01:15 +0400 From: Ivan Gerasimov ivan.gerasi...@oracle.com Subject: Re: RFR: [8036088] - Thread-unsafe strtok() is used to parse the list of overrides To: Christos Zoulas chris...@zoulas.com,OpenJDK Network Dev list net-dev@openjdk.java.net Message-ID: 5314b55b.9070...@oracle.com Content-Type: text/plain; charset=UTF-8; format=flowed Hi Christos! On 03.03.2014 20:52, chris...@zoulas.com wrote: On Mar 3, 8:32pm, ivan.gerasi...@oracle.com (Ivan Gerasimov) wrote: -- Subject: RFR: [8036088] - Thread-unsafe strtok() is used to parse the list | Hello! | | The strtok() function is used in | ./windows/native/sun/net/spi/DefaultProxySelector.c. | This function is not thread safe, so it may potentially cause a problem. | | The failure in this particular place would be very unlikely, because | this code should be executed only once during initialization. | Therefore, no regtest provided. | | The fix would be to use a thread-safe equivalent, which is strtok_s() | under Windows. | | Would you please help review this simple fix? | | BUGURL: https://bugs.openjdk.java.net/browse/JDK-8036088 | WEBREV: http://cr.openjdk.java.net/~igerasim/8036088/0/webrev/ Doesn't windows have strtok_r() IEEE Std 1003.1c-1995 (``POSIX.1''). MSDN does not refer to strtok_r(). Grepping the JDK code shows that strtok_s() is used in the windows-specific code. Sincerely yours, Ivan christos End of net-dev Digest, Vol 81, Issue 3 **
Re: RFR for bug JDK-8035633 TEST_BUG: java/net/NetworkInterface/Equals.java and some tests failed on windows intermittently
Hello Eric Would you please also correct the comment on the line above your change: s/seudo/pseudo/ The typo is in all three files. Sincerely yours, Ivan On 27.02.2014 12:18, Eric Wang wrote: On 2014/2/25 16:59, Alan Bateman wrote: On 25/02/2014 08:49, Eric Wang wrote: Hi Everyone, I'm working on the test bug https://bugs.openjdk.java.net/browse/JDK-8035633, There are 4 tests (one is in a closed test) failed due to NullPointerException. All tests failed at similar places if (isWindows ni.getDisplayName().contains(Teredo)), the root cause is the NetworkInterface may return null if no display name is available. so the fix is to make sure the display name is not null before calling the method contains(Teredo), something like if (isWindows displayName != null displayName.contains(Teredo)) Please let me know if you have any comment. The webrev will be sent after internal review. Yes, NetworkInterface's getDisplyName is allowed to return null and it looks like this wasn't taken into account when these tests were changed via JDK-8022963 to skip tunnels. Once you have a patch then net-dev would be the best list to bring it to. -Alan. Thanks Alan to suggest the correct alias! Below is the webrev for this bug, Can you or anyone else help to review the fix? http://cr.openjdk.java.net/~ewang/JDK-8035633/webrev.00/ http://cr.openjdk.java.net/%7Eewang/JDK-8035633/webrev.00/ There is one more closed test fixed, it will be sent in another mail. Thanks, Eric
Re: RFR [8023390] Test java/net/NetworkInterface/MemLeakTest.java failed
On 14.10.2013 14:28, Chris Hegarty wrote: I'm really not sure that the effort this test is going to is really necessary here ( to verify such a minor leak ). I'd be happy to see the test simply removed. Other opinions? I agree. I only regret that this method of measurement of native memory usage turns out to be unreliable. At first it seemed promising. Sincerely yours, Ivan -Chris. On 11/10/2013 20:24, Ivan Gerasimov wrote: On 10.10.2013 15:52, Ivan Gerasimov wrote: Hi Chris! I've run the test on JPRT and it took 84, 99 and 146 seconds on three different machines. When I run it locally in the virtualbox, it takes approximately 300 seconds. Please note that even though the number of iterations was increased by 3.5 times, now only one NetworkInterface is accessed during one iteration. Not 3.5, but of course 14 times. Silly arithmetic mistake. However the rest remains true. So on systems with many network interfaces the total time should even be less than before. Nevertheless I doubled the timeout. Sincerely yours, Ivan On 09.10.2013 19:57, Chris Hegarty wrote: Do you have a sense of how long this test runs for, on an average machine, with the extra iterations? -Chris. On 09/10/2013 09:24, Ivan Gerasimov wrote: Hello all! The MemLeakTest had been added with the fix for 8022584. Since that, the test was reported to fail intermittently, even though the leak was eliminated. I couldn't ever reproduce the failure even on the machines where the failure was detected. Here are the changes I propose: - Increase number of both warm-up and measured iterations, - Number of iterations now indicates how many times a single interface is probed. It used to probe all the interfaces that many times. - Increase the memory consumption threshold - Increase the timeout These should add some confidence that the failure of the test really indicates a memory leak. In addition to that, in the case of a failure the list of all the network interfaces is displayed, so there will be some more information about the environment. Here is the webrev: http://cr.openjdk.java.net/~igerasim/8023390/0/webrev/ Sincerely yours, Ivan Gerasimov
Re: RFR [8023390] Test java/net/NetworkInterface/MemLeakTest.java failed
Hi Chris! I've run the test on JPRT and it took 84, 99 and 146 seconds on three different machines. When I run it locally in the virtualbox, it takes approximately 300 seconds. Please note that even though the number of iterations was increased by 3.5 times, now only one NetworkInterface is accessed during one iteration. So on systems with many network interfaces the total time should even be less than before. Nevertheless I doubled the timeout. Sincerely yours, Ivan On 09.10.2013 19:57, Chris Hegarty wrote: Do you have a sense of how long this test runs for, on an average machine, with the extra iterations? -Chris. On 09/10/2013 09:24, Ivan Gerasimov wrote: Hello all! The MemLeakTest had been added with the fix for 8022584. Since that, the test was reported to fail intermittently, even though the leak was eliminated. I couldn't ever reproduce the failure even on the machines where the failure was detected. Here are the changes I propose: - Increase number of both warm-up and measured iterations, - Number of iterations now indicates how many times a single interface is probed. It used to probe all the interfaces that many times. - Increase the memory consumption threshold - Increase the timeout These should add some confidence that the failure of the test really indicates a memory leak. In addition to that, in the case of a failure the list of all the network interfaces is displayed, so there will be some more information about the environment. Here is the webrev: http://cr.openjdk.java.net/~igerasim/8023390/0/webrev/ Sincerely yours, Ivan Gerasimov
RFR [8023390] Test java/net/NetworkInterface/MemLeakTest.java failed
Hello all! The MemLeakTest had been added with the fix for 8022584. Since that, the test was reported to fail intermittently, even though the leak was eliminated. I couldn't ever reproduce the failure even on the machines where the failure was detected. Here are the changes I propose: - Increase number of both warm-up and measured iterations, - Number of iterations now indicates how many times a single interface is probed. It used to probe all the interfaces that many times. - Increase the memory consumption threshold - Increase the timeout These should add some confidence that the failure of the test really indicates a memory leak. In addition to that, in the case of a failure the list of all the network interfaces is displayed, so there will be some more information about the environment. Here is the webrev: http://cr.openjdk.java.net/~igerasim/8023390/0/webrev/ Sincerely yours, Ivan Gerasimov
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 perform memory leak
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 tweaking eg I think
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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.
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 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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
Thank you Michael! I'm working on the test. Chris, if it's not too late, I would like to include a regtest into the change. It will be ready in a few minutes and I'll send an updated webrev. Thanks, Ivan On 08.08.2013 17:51, Michael McMahon wrote: The patch looks good to me. I guess a regression test isn't feasible. So, the bug will be tagged noreg-hard Michael On 08/08/13 14:39, Ivan Gerasimov wrote: Thanks, David I've updated the webrev http://cr.openjdk.java.net/~igerasim/8022584/2/webrev/. On 08.08.2013 9:01, 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. It seemed to me that, JNU_ThrowOutOfMemoryError only throws an exception in a case one is not pending. But I don't mind to remove it, relaying on the correct implementation of GetStringUTFChars. Sincerely yours, Ivan 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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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. 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
