Re: RFR: JDK-8184770: JDWP support for IPv6
(added net-dev as suggested by Alan) Net gurus, please assist in reviewing socket-related code. New webrev: http://cr.openjdk.java.net/~amenkov/IPv6/webrev.01/ On 03/28/2019 07:44, Gary Adams wrote: Is there any documentation that needs to be updated along with the impl changes? created an issue for this: https://bugs.openjdk.java.net/browse/JDK-8221707 Would it make sense to support the preference properties? java.net.preferIPv4Stack java.net.preferIPv6Addresses Done (with new test for the functionality) Will the previous jdwp tests run with IPv6 or just the new additions? many tests use "localhost" (or empty string which has the same meaning) address. localhost can be resolved to IPv4 or IPv6 address, but by default IPv4 is used. So IPv6 will be used only on IPv6-only systems. Should probably have reviewers with networking expertise. core-libs(?) Do we know if IPv6 is enabled in our test infrastructure? Accordingly the logs IPv6 stack is enabled, but no external IPv6 address is assigned (i.e. the only IPv6 address is loopback - ::1). This is enough for testing. A stray "printf" statement in the initial webrev. socketTransport.c fixed. --alex On 3/27/19, 7:04 PM, Alex Menkov wrote: Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8184770 webrev: http://cr.openjdk.java.net/~amenkov/IPv6/webrev.00/ Main changes are in socketTransport.c - the code is updated to support both IPv4 and IPv6. Some details to simplify reviewing: - listening: - if IP address is specified (like 127.0.0.1 or ::1), connector listens only on this address; - for backward compatibility if no address (or "localhost") is specified, IPv4 is used (if supported by the host); - if "*" is specified (means "listen on all local interfaces"), dual mode socket is used to listen on both IPv6 and IPv4 addresses; - AllowedPeerInfo structure (for "allow" option) is updated to use IPv6 address/mask, support for IPv4 is implemented by using "mapped" IPv4 addresses; - attaching: agent resolves and tries to connect to all (IPv4 and IPv6) addresses, IPv4 are tried first; SocketListeningConnector.java/SocketTransportService.java are updated to support IPv6 addresses (the addresses may contain colons); new JdwpAttachTest.java/JdwpListenTest.java test that listening and attaching works for all available addresses (Ipv4 and IPv6) BasicJDWPConnectionTest.java was renamed to JdwpAllowTest.java (as it tests "allow" functionality), tests for mask (prefix length) functionality are added (for both IPv4 and IPv6); --alex
Re: RFR: JDK-8184770: JDWP support for IPv6
Hi Daniel, Chris,
Unfortunately docs are out-dated (I plan to update it in JDK13).
In JDK9
https://bugs.openjdk.java.net/browse/JDK-8041435
changed the behavior - empty address (i.e. only port is specified) means
"local connections only", "*" means "all available connections".
Then in JDK10 new "allow" option was introduced:
https://bugs.openjdk.java.net/browse/JDK-8061228
(it allows to specify list of addresses/subnets to accept connections from).
--alex
On 04/01/2019 06:40, Chris Hegarty wrote:
On 01/04/2019 10:51, Daniel Fuchs wrote:
Hi Arthur,
Not directly related to Alex's original question but...
On 30/03/2019 00:03, Arthur Eubanks wrote:
We have some ipv6 patches as well in this area as well (as well as
other patches to support ipv6 only environments) that we're trying to
upstream. I don't understand the code myself, but it might be useful
to take a look:
http://cr.openjdk.java.net/~aeubanks/jdwpipv6/webrev.00/index.html
SocketTransportService.java:
On my machine at least, InetAddress.getByName("localhost") and
InetAddress.getLocalHost() are quite different:
InetAddress.getByName("localhost") will return the loopback (127.0.0.1)
InetAddress.getLocalHost() returns the local hostname (dhcp-XXX-XXX...)
If I'm not mistaken your changes in HostPort would transform something
that previously returned the loopback (no host specified) with something
that returns the local host name ("*" specified).
So I'm not sure these changes are quite right.
Maybe Alex will be able to confirm.
We need to consult the documentation for the command-line tools that
exercise this code, or the higher-level API spec for the expected
behavior of `*`, port-only specified, etc.
-Chris.
Re: RFR: JDK-8184770: JDWP support for IPv6
Hi Chris, On 04/01/2019 04:50, Chris Hegarty wrote: Alex, On 29/03/2019 22:07, Alex Menkov wrote: (added net-dev as suggested by Alan) Net gurus, please assist in reviewing socket-related code. New webrev: http://cr.openjdk.java.net/~amenkov/IPv6/webrev.01/ Specifically on SocketTransportService.java. What Arthur has proposed is better ( changing to lastIndexOf alone is not sufficient ). Or is your assumption that the IPv6 literal is not enclosed in square brackets? I didn't know about enclosing IPv6 in square brackets, but looks like that's standard way to alleviate conflict between IPv6 address and colon as port separator. Will update the fix to handle them in both JDI connectors (SocketTransportService.java) and debugger agent (socketTransport.c) If keeping Arthur's `static class HostPort` please make the fields final. >> Would it make sense to support the preference properties? >> java.net.preferIPv4Stack >> java.net.preferIPv6Addresses I'm not sure about this, especially given the property name prefixes. I need to think a little more on it. In the initial version of the fix I didn't check the properties. The rationale here is backward compatibility - is address is empty, old debuggers tries to connect to IPv4 address only. But handling this properties we will better handle clients with properties set (as jdb or any other debugger which uses JDI connectors are affected by the properties). BTW fix provided by Arthur implements listening on localhost differently - it creates several sockets and binds them to both IPv4 and IPv6 addresses. But the problem here (and that's the reason I decide to not implement it this way) - how to handle the case when we successfully bind on one address (for example IPv4), but fail to bind on other (for example the port is busy for IPv6 stack). Arthur's version just fail in the case (i.e. the whole Java process terminates) and I don't think this is good behavior. There is quite a bit of new native code, is it possible to rewrite any of this in Java, e.g. reading of the system properties ( if that is to remain )? socketTransport.c is a debugger agent which is completely native. --alex -Chris.
Re: RFR: JDK-8184770: JDWP support for IPv6
On 04/01/2019 05:02, Chris Hegarty wrote: On 01/04/2019 12:50, Chris Hegarty wrote: ... Specifically on SocketTransportService.java. What Arthur has proposed is better ( changing to lastIndexOf alone is not sufficient ). Or is your assumption that the IPv6 literal is not enclosed in square brackets? More specifically, what syntax are you proposing to pass IPv6 addresses from the command line? For JDI connectors (SocketTransportService.java) there are 2 separate connector arguments: "localAddress" and "port". For JDWP agent (socketTransport.c) arguments are provided as a single command line argument: java -agentlib:jdwp=transport=dt_socket,server=y,address=: MyApp --alex -Chris
Re: RFR: JDK-8184770: JDWP support for IPv6
Hi Daniel,
As far as I see you are right.
Attach to address with empty hostname will try to connect to "external"
address instead of loopback.
--alex
On 04/01/2019 02:51, Daniel Fuchs wrote:
Hi Arthur,
Not directly related to Alex's original question but...
On 30/03/2019 00:03, Arthur Eubanks wrote:
We have some ipv6 patches as well in this area as well (as well as
other patches to support ipv6 only environments) that we're trying to
upstream. I don't understand the code myself, but it might be useful
to take a look:
http://cr.openjdk.java.net/~aeubanks/jdwpipv6/webrev.00/index.html
SocketTransportService.java:
On my machine at least, InetAddress.getByName("localhost") and
InetAddress.getLocalHost() are quite different:
InetAddress.getByName("localhost") will return the loopback (127.0.0.1)
InetAddress.getLocalHost() returns the local hostname (dhcp-XXX-XXX...)
If I'm not mistaken your changes in HostPort would transform something
that previously returned the loopback (no host specified) with something
that returns the local host name ("*" specified).
So I'm not sure these changes are quite right.
Maybe Alex will be able to confirm.
best regards,
-- daniel
Re: RFR: JDK-8184770: JDWP support for IPv6
Updated webrev: http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/ - added support for addresses enclosed in square brackets; - updated SocketTransportService.java to handle empty hostname the same way as JDWP agent (listen/attach to loopback address); Has to update nsk/jdi/ListeningConnector/startListening/startlis001.java (all other com/sun/jdi, nsk/jdi, nsk/jdwp, nsk/jdb test are passed). --alex On 04/01/2019 11:21, Alex Menkov wrote: Hi Chris, On 04/01/2019 04:50, Chris Hegarty wrote: Alex, On 29/03/2019 22:07, Alex Menkov wrote: (added net-dev as suggested by Alan) Net gurus, please assist in reviewing socket-related code. New webrev: http://cr.openjdk.java.net/~amenkov/IPv6/webrev.01/ Specifically on SocketTransportService.java. What Arthur has proposed is better ( changing to lastIndexOf alone is not sufficient ). Or is your assumption that the IPv6 literal is not enclosed in square brackets? I didn't know about enclosing IPv6 in square brackets, but looks like that's standard way to alleviate conflict between IPv6 address and colon as port separator. Will update the fix to handle them in both JDI connectors (SocketTransportService.java) and debugger agent (socketTransport.c) If keeping Arthur's `static class HostPort` please make the fields final. >> Would it make sense to support the preference properties? >> java.net.preferIPv4Stack >> java.net.preferIPv6Addresses I'm not sure about this, especially given the property name prefixes. I need to think a little more on it. In the initial version of the fix I didn't check the properties. The rationale here is backward compatibility - is address is empty, old debuggers tries to connect to IPv4 address only. But handling this properties we will better handle clients with properties set (as jdb or any other debugger which uses JDI connectors are affected by the properties). BTW fix provided by Arthur implements listening on localhost differently - it creates several sockets and binds them to both IPv4 and IPv6 addresses. But the problem here (and that's the reason I decide to not implement it this way) - how to handle the case when we successfully bind on one address (for example IPv4), but fail to bind on other (for example the port is busy for IPv6 stack). Arthur's version just fail in the case (i.e. the whole Java process terminates) and I don't think this is good behavior. There is quite a bit of new native code, is it possible to rewrite any of this in Java, e.g. reading of the system properties ( if that is to remain )? socketTransport.c is a debugger agent which is completely native. --alex -Chris.
Re: RFR: JDK-8184770: JDWP support for IPv6
updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/
changes (vs webrev.02) are non-functional (added/edited comments, code
reformatting, renaming convertIpv4ToIpv6 function to convertIPv4ToIPv6,
renaming env variable to jniEnv (env is already used in one of the
functions)).
About pass/preferredAddressFamily conditions - there is no "logical xor"
in C/C++. Also I think that the current condition is clearer.
--alex
On 04/11/2019 17:18, serguei.spit...@oracle.com wrote:
Hi Alex,
Great debugging feature!
While I'm still reading all the details, could you, please, fix some
minor format issues?
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html
+ * If host is a literal IPv6 address, it may be in square
brackets. Extra space before "square".
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html
I'd suggest to unify comments before functions:
- start comment with a capital latter and ended with a dot
- use comment format like this:
/*
*/
Examples of comments that need this change:
262 /* result must be release with dbgsysFreeAddrInfo */ => /* * Result
must be release with dbgsysFreeAddrInfo. */
325 // input is sockaddr just because all clients have it =>
/* * Input is sockaddr just because all clients have it. */
1129 /* reads boolean system value,
1130 * sets *result to trueValue if the ptoperty is "true",
1131 * to falseValue if the property if "false",
1132 * doesn't change *result if the property is not set or failed to read.
1133 */ => /* * Read boolean system value andset result to: * -
trueValue if the property is "true"
* - falseValue if the property is "false" * * Return JNI_OK if result is set,
return JNI_ERR otherwise.
*/
. . .
293 * use IPv6 socket (to accept IPv6 and mapped Ipv4),...
342 * (with AF_INET6 Ipv4 addresses are not parsed even with AI_V4MAPPED
and AI_ALL flags) ...345 hints.ai_family = AF_UNSPEC; // IPv6 or mapped
Ipv4 ... 360 } else { // Ipv4 address Replace Ipv4 with IPv4 for
unification with IPv6
For unification replace: convertIpv4ToIpv6 => convertIPv4ToIPv6
297 hints.ai_flags |= AI_PASSIVE
298 | (allowOnlyIPv4 ? 0 : AI_V4MAPPED | AI_ALL); Better to have just
one line
1135 JNIEnv* env, . . . 1165 JNIEnv* jniEnv = NULL;
A suggestion is to use the same name for JNIEnv*:
1135 JNIEnv* jni, . . .
1165 JNIEnv* jni = NULL;
Reformat:
608 if ((pass == 0 && ai->ai_family == preferredAddressFamily) 609 ||
(pass == 1 && ai->ai_family != preferredAddressFamily)) and
828 if ((pass == 0 && ai->ai_family == preferredAddressFamily) 829 ||
(pass == 1 && ai->ai_family != preferredAddressFamily)) => if ((pass ==
0 && ai->ai_family == preferredAddressFamily) || (pass == 1 &&
ai->ai_family != preferredAddressFamily))
Even better, replace it with logical XOR:
if ((pass == 0 ^^ ai->ai_family == preferredAddressFamily)
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jdi/ListeningConnector/startListening/startlis001.java.frames.html
102 /* Check that listening address returned by
ListeningConnector.startListening()
103 * matches the address which was set via connector's arguments.
104 * Empty host address causes listening for local connections only
(loopback interface)
105 * */ Dot is missed at the end. Replace "* */" with "*/".
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/test/jdk/com/sun/jdi/JdwpAllowTest.java.frames.html
162 // generate allow address by changing random bit in the local address
163 // and calculate 2 masks (prefix length) - one is matches original
local address
164 // and another doesn't. Replace with /* style of comment.
249 positiveTest("PositiveMaskTest(" + test.localAddress + ")",
test.allowAddress + "/" + test.prefixLengthGood);
250 positiveTest("NegativeMaskTest(" + test.localAddress + ")",
test.allowAddress + "/" + test.prefixLengthBad);
A suggestion to move second argument to additional line:
positiveTest("PositiveMaskTest(" + test.localAddress + ")",
test.allowAddress + "/" + test.prefixLengthGood);
positiveTest("NegativeMaskTest(" + test.localAddress + ")",
test.allowAddress + "/" + test.prefixLengthBad);
Thanks,
Serguei
On 4/2/19 4:14 PM, Alex Menkov wrote:
Updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/
- added support for addresses enclosed in square brackets;
- updated SocketTransportService.java to handle empty hostname the
same way as JDWP agent (listen/attach to loopback address);
Has to update nsk/jdi/List
Re: RFR: JDK-8184770: JDWP support for IPv6
Hi Serguei,
updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/
Fixed everything except #7 (for consistency - in the file if function
arguments spread on several lines, opening curly bracket is placed to
separate line to separate function declaration from the function body).
--alex
On 04/12/2019 18:52, serguei.spit...@oracle.com wrote:
Hi Alex,
Thank you for the update!
One more round of minor formatting change requests for better
readability. :)
#1:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html
+ port = Integer.decode(hostPort.substring(splitIndex+1));
. . .
+ } else if (hostPort.charAt(0) == ‘[’ &&
hostPort.charAt(splitIndex-1) == ‘]’) {
Need spaces around ‘+’ and ‘-’ signs.
#2:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c.udiff.html
+ //make the socket a dual mode socket
missed space at the start of comment
Now, comments for:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html
#3:
276 /* check for host:port or port */
277 colon = strrchr(address, ‘:’);
278 port = (colon == NULL ? address : colon + 1);
279 /* ensure the port is valid (getaddrinfo allows port to be empty) */
280 if (getPortNumber(port) < 0) {
#4:
298 hints.ai_family = allowOnlyIPv4 ? AF_INET : AF_INET6;
299 hints.ai_flags |= AI_PASSIVE | (allowOnlyIPv4 ? 0 :
AI_V4MAPPED | AI_ALL);
300
301 } else {
#5: Replace “fills” with “fills in” in:
341 * Parses address (IPv4 or IPv6), fills result by parsed address.
383 * Parses prefix length from buffer (integer value), fills result
with corresponding net mask.
485 * Parses ‘allow’ argument (fills list of allowed peers (global
_peers variable)).
#6:
410 // generate mask for prefix length
411 memset(result, 0, sizeof(*result));
412 // prefixLen <= 128, so we won’t go over result’s size
413 for (int i = 0; prefixLen > 0; i++, prefixLen -= :sunglasses: {
#7:
623 socketTransport_startListening(jdwpTransportEnv* env, const char*
address,
624 char** actualAddress)
625 {
. . . .
1173 static int readBooleanSysProp(int *result, int trueValue, int
falseValue,
1174 JNIEnv* jniEnv, jclass sysClass, jmethodID getPropMethod, const
char *propName)
1175 {
Move ‘{’ to the end of 624 and 1174. (edited)
#8:
1176 jstring value;
1177 jstring name = (*jniEnv)->NewStringUTF(jniEnv, propName);
1178 if (name == NULL) {
. . . .
1259 } while (0);
1260 if (jniEnv != NULL && (*jniEnv)->ExceptionCheck(jniEnv)) {
Thanks!
Serguei
On 4/12/19 4:58 PM, Alex Menkov wrote:
updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.03/
changes (vs webrev.02) are non-functional (added/edited comments, code
reformatting, renaming convertIpv4ToIpv6 function to
convertIPv4ToIPv6, renaming env variable to jniEnv (env is already
used in one of the functions)).
About pass/preferredAddressFamily conditions - there is no "logical
xor" in C/C++. Also I think that the current condition is clearer.
--alex
On 04/11/2019 17:18, serguei.spit...@oracle.com wrote:
Hi Alex,
Great debugging feature!
While I'm still reading all the details, could you, please, fix some
minor format issues?
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdi/share/classes/com/sun/tools/jdi/SocketTransportService.java.udiff.html
+ * If host is a literal IPv6 address, it may be in
square brackets. Extra space before "square".
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.02/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.frames.html
I'd suggest to unify comments before functions:
- start comment with a capital latter and ended with a dot
- use comment format like this:
/*
*/
Examples of comments that need this change:
262 /* result must be release with dbgsysFreeAddrInfo */ => /* *
Result must be release with dbgsysFreeAddrInfo. */
325 // input is sockaddr just because all clients have it =>
/* * Input is sockaddr just because all clients have it. */
1129 /* reads boolean system value,
1130 * sets *result to trueValue if the ptoperty is "true",
1131 * to falseValue if the property if "false",
1132 * doesn't change *result if the property is not set or failed to
read.
1133 */ => /* * Read boolean system value andset result to: * -
trueValue if the property is "true"
* - falseValue if the property is "false" * * Return JNI_OK if
result is set, return JNI_ERR otherwise.
*/
. . .
293 * use IPv6 socket (to accept IPv6 and mapped Ipv4),...
342 * (with AF_INET6 Ipv4 addresses are not parsed even with
AI_V4MAPPED and AI_A
Re: PING: Re: RFR: JDK-8184770: JDWP support for IPv6
Hi Chris, Serguei,
Updated webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.05/
CSR (approved):
https://bugs.openjdk.java.net/browse/JDK-8223104
Changes (vs. webrev.04):
- setsockopt(IPV6_V6ONLY) was moved from Win-specific code to shared
setOptionsCommon function (in socketTransport.c)
the value by default is "true" on Windows and is "false" on Unix, but
it can be overridden, so lets set it explicitly;
- a comment why the result of setsockopt(IPV6_V6ONLY) is ignored is added;
- added some comments as per Serguei request.
About scopes support - I thought that the functionality is not important
for debugger, but I can implement it (I'd prefer to do it by separate RFE).
--alex
On 05/11/2019 03:39, Chris Hegarty wrote:
On 7 May 2019, at 19:40, serguei.spit...@oracle.com wrote:
Hi guys,
We need a couple of partial reviews for this enhancement:
- From the net-dev to check IPv6-addresses related part.
It does not need to be a thorough review.
We need another pair of eyes to check for obvious typos or misses.
Included Chris H. to mailing list in hope for some assistance.
(Chris, we need some help to find one of the IPv6 experts.)
- From the serviceability-dev to check if compatibility
is not broken and nothing is missed.
This includes part of code that is not directly involved
into manipulations with the IPv4/IPv6 addresses.
The related spec update is tracked by a sub-task:
https://bugs.openjdk.java.net/browse/JDK-8221707
Related CSR:
https://bugs.openjdk.java.net/browse/JDK-8223104
The latest webrev:
http://cr.openjdk.java.net/~amenkov/IPv6/webrev.04/
src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c
223 if (domain == AF_INET6) {
224 int off = 0;
225 // make the socket a dual mode socket
226 setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, (char *)&off,
sizeof(off));
227 }
228 }
This code is fine, but maybe you want to expand the comment to
mention that the setsockopt may fail if IPv4 is not supported. And
that’s OK.
I cannot find a similar setting of IPV6_V6ONLY for the unix
equivalent. Why not, or where can it be found?
src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c
There is a lot of native code here, I skimmed over it, seems
reasonable. There is an obvious lack of any reference to IPv6
scopes. Can the address given to bind be an IPv6 link-local?
If so, then the scope will need to be parsed / set appropriately.
( seems that the new test skips all scoped addresses? )
-Chris.
Re: RFR: JDK-8224028: loop initial declarations introduced by JDK-8184770 (jdwp)
On 05/16/2019 06:41, Ao Qi wrote: Hi Serguei, I saw your email [1], but I didn't receive it yet. Thanks for your review! I updated: http://cr.openjdk.java.net/~aoqi/8224028/webrev.01/ Looks good. --alex On Thu, May 16, 2019 at 8:30 PM David Holmes wrote: What compiler was used here? We shouldn't be using anything that doesn't handle loop variable declarations! The compiler I used is gcc 4.8.5. This machine is used for testing jdk/jdk for months. As far as I remember, loop variable declarations issue never been found. If gcc 4.8.5 is not a supported compiler, I think we should update building doc [2]. Thanks, David On 16/05/2019 7:41 pm, Daniel Fuchs wrote: Hi Ao Qi, I'm adding serviceability-dev, since this is for jdwp. The proposed changes look good to me - but please get someone from the serviceability team to review this. Thanks Daniel! Cheers, Ao Qi [1] https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-May/028097.html [2] https://hg.openjdk.java.net/jdk/jdk/raw-file/17926213de55/doc/building.html#gcc best regards, -- daniel On 16/05/2019 08:41, Ao Qi wrote: Hi, I found build is failed on CentOS 7.6, because of loop initial declarations. Could I please get reviews for this? Bug: https://bugs.openjdk.java.net/browse/JDK-8224028 Webrev: http://cr.openjdk.java.net/~aoqi/8224028/webrev.00/ Tested: linux-x86_64-server-release tier1 Thanks, Ao Qi
Re: RFR(s): Improving performance of Windows socket connect on the loopback adapter
Hi Nikola, One note. src\java.base\windows\native\libnet\net_util_md.h IN6_IS_ADDR_V4MAPPED_LOOPBACK considers only 127.0.0.1 as loopback address, but AFAIR it's the whole block 127.0.0.0/8 and 127.0.0.1 is just the most common used address. --alex On 07/22/2020 07:14, Nikola Grcevski wrote: Thanks again Alan. My apologies for the delayed response, I was away most of yesterday. I've updated the webrev with the change as requested: http://cr.openjdk.java.net/~adityam/nikola/fast_connect_loopback_3/ None of us here at Microsoft have contributor status yet, so I'll need a sponsor to get this change merged. Best, Nikola -Original Message- From: Alan Bateman Sent: July 21, 2020 11:19 AM To: Nikola Grcevski ; net-dev@openjdk.java.net Subject: Re: RFR(s): Improving performance of Windows socket connect on the loopback adapter On 21/07/2020 02:34, Nikola Grcevski wrote: Hi Alan and Bernd, Thanks again for the code review of my changes and the suggestions! Please find the updated webrev here: https://nam06.safelinks.protection.outlook.com/?url=http:%2F%2Fcr.open jdk.java.net%2F~adityam%2Fnikola%2Ffast_connect_loopback_2%2F&data =02%7C01%7CNikola.Grcevski%40microsoft.com%7C4d89c178d8a147bc6e1708d82 d895c25%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63730941526294228 9&sdata=75i%2BHx1QXiqydrPd49Z4UbiO83SSLCxgXBrzovGDs%2FY%3D&res erved=0 I decided to explicitly check so_rv for success consistently in the two places. It feels safer against future changes to the internal implementation of getsockopt. I left the JNICALL and jint to match the other similar functions in the net helper functions file as before. It sounds like future clean-up will simplify this, but if I misunderstood from your comments please let me know and I'll update accordingly. I think this version looks okay, except that I'd prefer if the if the expression at L244-246 were on one line, not split over there. Do you have a sponsor to push this? -Alan
RFR: 8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach
The fix also partially fixes JdwpAttachTest failures (JDK-8253940).
The failures are caused by network configuration changes during test execution
("global" IPv6 addresses disappears from interface).
To minimize chances of intermittent failures like this java.net tests use only
link-local addresses whenever possible.
The fix does the same for JDI tests
(Utils.getAddressesWithSymbolicAndNumericScopes is used by JdwpListenTest and
JdwpAttachTest)
-
Commit messages:
- test only IPv6 link-local addresses
Changes: https://git.openjdk.java.net/jdk/pull/2633/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2633&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8224775
Stats: 14 lines in 1 file changed: 3 ins; 0 del; 11 mod
Patch: https://git.openjdk.java.net/jdk/pull/2633.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2633/head:pull/2633
PR: https://git.openjdk.java.net/jdk/pull/2633
Re: RFR: 8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach
On Thu, 18 Feb 2021 21:43:00 GMT, Alex Menkov wrote:
> The fix also partially fixes JdwpAttachTest failures (JDK-8253940).
> The failures are caused by network configuration changes during test
> execution ("global" IPv6 addresses disappears from interface).
> To minimize chances of intermittent failures like this java.net tests use
> only link-local addresses whenever possible.
> The fix does the same for JDI tests
> (Utils.getAddressesWithSymbolicAndNumericScopes is used by JdwpListenTest
> and JdwpAttachTest)
Added serviceability and net as this is JDI tests, but the issue is
networking-related
-
PR: https://git.openjdk.java.net/jdk/pull/2633
Re: RFR: 8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach
On Mon, 22 Feb 2021 17:34:03 GMT, Daniel Fuchs wrote: > > > I don't see any specific issue with the proposed change but I don't know the > JDWP tests enough to provide more feedback than that. Do you have special > test cases for the IPv6 loopback? AFAIU this code here will filter it out? Good catch. IPv6 loopback addresses shouldn't be filtered out. I'll update PR after re-testing - PR: https://git.openjdk.java.net/jdk/pull/2633
Re: RFR: 8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach [v2]
> The fix also partially fixes JdwpAttachTest failures (JDK-8253940).
> The failures are caused by network configuration changes during test
> execution ("global" IPv6 addresses disappears from interface).
> To minimize chances of intermittent failures like this java.net tests use
> only link-local addresses whenever possible.
> The fix does the same for JDI tests
> (Utils.getAddressesWithSymbolicAndNumericScopes is used by JdwpListenTest
> and JdwpAttachTest)
Alex Menkov has updated the pull request incrementally with one additional
commit since the last revision:
include loopback addresses in testing
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2633/files
- new: https://git.openjdk.java.net/jdk/pull/2633/files/96176a61..4d59abf6
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2633&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2633&range=00-01
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/2633.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2633/head:pull/2633
PR: https://git.openjdk.java.net/jdk/pull/2633
Integrated: 8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach
On Thu, 18 Feb 2021 21:43:00 GMT, Alex Menkov wrote:
> The fix also partially fixes JdwpAttachTest failures (JDK-8253940).
> The failures are caused by network configuration changes during test
> execution ("global" IPv6 addresses disappears from interface).
> To minimize chances of intermittent failures like this java.net tests use
> only link-local addresses whenever possible.
> The fix does the same for JDI tests
> (Utils.getAddressesWithSymbolicAndNumericScopes is used by JdwpListenTest
> and JdwpAttachTest)
This pull request has now been integrated.
Changeset: 104a2628
Author:Alex Menkov
URL: https://git.openjdk.java.net/jdk/commit/104a2628
Stats: 14 lines in 1 file changed: 3 ins; 0 del; 11 mod
8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach
Reviewed-by: sspitsyn, dfuchs
-
PR: https://git.openjdk.java.net/jdk/pull/2633
Re: RFR: 8280010: Remove double buffering of InputStream for Properties.load
On Mon, 10 Jan 2022 20:46:36 GMT, Andrey Turbanov wrote: > `Properties.load` uses `java.util.Properties.LineReader`. LineReader already > buffers input stream. Hence wrapping InputStream in BufferedInputStream is > redundant. Marked as reviewed by amenkov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7021
