RFR: 8223145: [teststabilization] Replace wildcard address with loopback or local host in test - part 1

[Daniel Fuchs]

Hi Mark,

Welcome back to this  list :-)

On 30/04/2019 23:58, mark sheppard wrote:
> <LE NOISE> ​
> Hi Daniel,​
> ​
> interesting set of changes -​
> But could it be the case that, for some tests, you might change the 
> operational semantics of a test, when​
> this applying this change.

I did try to avoid that. Of course I am aware that binding
to the loopback vs binding to wildcard vs binding to the
external host IP means that different code paths will
be involved. In some cases I even went and checked the
history of the test to verify that the use of the wildcard
wasn't made on purpose. I didn't do it for all the tests,
but GetLocalAddress is one I checked - for I had similar
concerns.

> For example, in the case of GetLocalAddress.​
> The original is to use a wild card for the server, and a directed 
> address for the ​
> client connect. Thus the server is listening INADDR_ANY or the IPv6 
> equivalent, accepts​
> a connection, then a check is made to see that socket created is bound 
> to the same address​
> as that specified in the clients connect request.​
> ​
> If the server socket is  bound to a specific address then the socket 
> created by the accept​
> will (always)  have the same address as that of initial server socket 
> (listener).​

OK. But the first bug tested here was that the returned address
had its byte[] in the right byte order - as it seemed there was
a bug on some platform where the local address returned by
the accepted socket had its bytes in the reversed order.
The second bug tested is that getLocalHost() doesn't crash when
java.net.preferIPv4Stack=true, and the third, is about
testBindNull() which has remained unchanged.

> As the tests are run with 3 different java.net property settings, it is 
> worth asking if the tests behaves as expected​
> for the case when neither java.net property is set.

Well, I haven't seen the test failing since I fixed it - which
is a good sign - so all three @run are passing, including the
one where no property is set, on all platforms that we test.
​
> AFAIK, on windows it shows some ambiguity and difference to the default 
> setting described in​
> [1]. The default run returns IPV4 addresses, creates AF_INTET sockets 
> and binds to IPv4 addresses.​
> This is possibly at odds with the default property settings are 
> described in the docs.​

hmmm... OK.

> java.net.preferIPv4Stack (default: false)​
> If IPv6 is available on the operating system the underlying native 
> socket will be, by default,​
> an IPv6 socket which lets applications connect to, and accept 
> connections from, both IPv4 and IPv6 hosts. ​
> This implies that the first test run, i.e. default property settings, 
> should be an AF_INET6 socket as in new ServerSocket​
> should be bound to the IPv6 unspecified address.​

Yes.

> While​
> java.net.preferIPv6Addresses (default: false)​
> When dealing with a host which has both IPv4 and IPv6 addresses, ​
> and if IPv6 is available on the operating system, the default ​
> behavior is to prefer using IPv4 addresses over IPv6 ones.​

Yes.
  ​
> This implies that the InetAddress.getLocalHost() should in the default case​
> return an IPv4 address.​

Yes. When java.net.preferIPv6Addresses is not set or not true.
  ​
> The client socket connect with IPv4 address to IPv6 wild card resulting 
> in an IPv4 mapped​
> binding in the socket created in the accept. This would mean the first 
> run should fail.​

It didn't fail either before or after my fix though. I mean - it did
fail before my fix but only intermittently and in timeout.

> In any case a test using a server socket listening on a wildcard 
> (INADDR_ANY) address has possibly​
> slightly different semantics than a server socket listening on a 
> specific host address.​

Agreed. And there must be tests that bind to the wildcard address,
if only to verify that binding to the wildcard works. But given
that binding to the wildcard is a proven source of instability, then
I believe that tests that do not explicitly need to bind to the
wildcard should not.

If we have only a limited amount of tests, clearly identified,
that need to use the wildcard address and do - and fail
intermittently - then that's no perfect but it can be
acceptable.
If the majority of the tests are susceptible to fail
randomly and intermittently then that's not acceptable.
Especially if we need to make sure that new changes to the code
base are not introducing any subtle race conditions.

In the case of GetLocalAddress then I'm confident that the
original test could have been written as it is after my
changes: prompted by your question I managed to track down
the SCCS history and see the ntohl() call that was added
to PlainSocketImpl.c.

best regards,

-- daniel


> ​
> regards
> Mark
> ​
> </LE NOISE>​
> ​
> [1] 
> https://docs.oracle.com/javase/8/docs/api/java/net/doc-files/net-properties.html
> 
> 
> ------------------------------------------------------------------------
> *From:* net-dev <net-dev-bounces at openjdk.java.net> on behalf of Daniel 
> Fuchs <daniel.fuchs at oracle.com>
> *Sent:* Tuesday 30 April 2019 11:16
> *To:* OpenJDK Network Dev list
> *Subject:* RFR: 8223145: [teststabilization] Replace wildcard address 
> with loopback or local host in test - part 1
> Hi,
> 
> Please find below a patch for:
> 
> 8223145: [teststabilization] Replace wildcard address with loopback
>            or local host in test - part 1
> https://bugs.openjdk.java.net/browse/JDK-8223145
> 
> http://cr.openjdk.java.net/~dfuchs/webrev_8223145/webrev.00/
> 
> This is the first in a series of patches that will try to
> address intermittent failures which are sometime observed
> when tests use the wildcard address to bind their test servers.
> 
> These changes are IpV6-only friendly.
> 
> best regards,
> 
> -- daniel