RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

Hi Severin,

Thanks for looking into this!

On 25/02/2019 15:31, Severin Gehwolf wrote:
> 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.

Thanks - I experimented with similar changes too.

>> 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?

Yes - well agreed - long history might have been a bit exaggerated.

>> 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?

Experience - and the fact that I saw it fail in timeout on my
local machine (albeit intermittently).
The test system runs tests with many different configurations,
some of them with various combination like -Xcomp or -Xint
which tend to have some measurable effects on JVM startup.
Tests that do compilation or start sub-processes are often
more sensitive to these option sets.
Usually, I would recommend not setting the timeout at all -
unless proven that the default jtreg timeout is too short.
Especially for those cases where the test is not supposed to
timeout.

>> 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].

Thanks.

>> 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

OK - thanks. Not sure how much stable that would be. Ideally we
should use port 0 - but then retrieving the actual port might
not be easy. Let's wait until we see the test fail.

> 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.

Network tests that use "localhost" and listen to 0.0.0.0 are
often more prone to intermittent failures. I think this is due
to the policy of reusing addresses and ports of the
underlying OS. This test doesn't bind to the wildcard address
however - so we can hope it will not fall into this category.


>> 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?

I believe so.

>> 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'"

Well - I'm going to send your patch through our internal
test system - I'll let you know of the result.

best regards,

-- daniel


> 
> 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
>>>
>