Hi Daniel, Thanks for the review!
Latest webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/webrev/ On Fri, 2019-02-22 at 18:31 +0000, Daniel Fuchs wrote: > Hi Severin, > > Did you manage to make the test pass locally? Yes. Here is the latest run: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/JMXInterfaceBindingTest.jtr FWIW, it passed locally with the v1 change proposed. But then again, it didn't do what it was intended for: Try to bind to multiple host:port pairs on a single system. This didn't work prior JDK-6425769 as it would bind to *all* interfaces. The updated webrev tries to remediate this. > This test has had a long history of failing, and I've come > to suspect that it might be flawed. If it has a history of failing, why aren't there any bugs for it? How did it fail? Are you referring to JDK-8145982 and JDK-8146015? > First - it has /timeout=5. I believe that's much too short. > It immediately failed the first time I ran it locally on my machine. I'm not sure we need a larger timeout. It's 20 seconds on my machine which seems more than enough. What makes you think we'd need to get this increased and it would actually do anything for test stability? > Then it uses well known ports. That's bad. There's no guarantee > that these ports will be free. Fair enough. Fixed in the latest webrev. Now picks a random port in range [9100, 9200]. > Then - it only use addresses configured for "localhost". > > Can there ever be more than one such address? Yes, a multi-homed computer could use this for example this in /etc/hosts on Linux: 192.168.1.18 localhost 192.168.122.1 localhost Looks like this on my system: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/JMXInterfaceBindingTest-multihome.jtr > If there is more > than one, I suspect the test will fail: I believe the subprocess > started by the test would need the additional > -Djava.rmi.server.hostname=<adddress> That's needed for 127.0.0.1 (loopback) support, so I've added it. Other IP addresses don't seem to need it. So the test does not need it per se, but it's useful so that more configs can actually test this. > on the command line. > And even with that - it might still fail if those two addresses > can't be bound simultaneously on the same port. No. Socket addresses are unique per host:port pair. If that wasn't possible, the whole test would be pointless. > Then why are loopback addresses filtered out? Because of two > things I guess: > > - if several loopback are defined, I'm not sure you can > bind them on the same port (and it might be difficult to > figure that out) > > - if you bind to the loopback address, then you most probably > need to start the subprocess with > -Djava.rmi.server.hostname=<loopback-adddress> Yes. I've tried to avoid those issues by allowing one non-loopback address and 127.0.0.1 explicitly. Is that a reasonable assumption? > If we don't see this test failing, I suspect this is because > it is either never selected, or always passes trivially. > If we start selecting it - or have a configuration where it > doesn't trivially pass, I suspect it will fail. My bet is on it passing trivially with: "Ignoring manual test since no more than one IPs are configured for 'localhost'" Then again, I don't know about Oracle's test selection settings. Thanks, Severin > > > On 22/02/2019 15:04, Severin Gehwolf wrote: > > Hi! > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8219585 > > > > Could somebody please review this trivial testbug. For a config like > > [1] the logic prior JDK-8145982 returned this list for > > getAddressesForLocalHost(): > > > > [localhost/127.0.0.1, localhost/192.168.1.18, localhost/0:0:0:0:0:0:0:1] > > > > Post JDK-8145982, getAddressesForLocalHost() returns: > > > > [localhost/192.168.1.18] > > > > The fix is trivial. Just adjust the condition for as to when the test > > should actually trivially pass: > > > > diff --git > > a/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java > > b/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java > > --- > > a/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java > > +++ > > b/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java > > @@ -176,8 +176,8 @@ > > } > > > > public static void main(String[] args) { > > - List<InetAddress> addrs = getAddressesForLocalHost(); > > - if (addrs.size() < 2) { > > + List<InetAddress> addrs = getNonLoopbackAddressesForLocalHost(); > > + if (addrs.size() < 1) { > > System.out.println("Ignoring manual test since no more than > > one IPs are configured for 'localhost'"); > > return; > > } > > @@ -186,7 +186,7 @@ > > System.out.println("All tests PASSED."); > > } > > > > - private static List<InetAddress> getAddressesForLocalHost() { > > + private static List<InetAddress> getNonLoopbackAddressesForLocalHost() > > { > > > > try { > > return NetworkInterface.networkInterfaces() > > > > > > Testing: Manual testing on local setup. jdk/submit (currently running) > > > > Thanks, > > Severin > > > > > > [1] $ grep localhost /etc/hosts | grep -v '::' > > 127.0.0.1 localhost localhost.localdomain localhost4 > > localhost4.localdomain4 > > 192.168.1.18 localhost > >
